You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by necouchman <gi...@git.apache.org> on 2017/08/16 16:01:26 UTC
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
GitHub user necouchman opened a pull request:
https://github.com/apache/incubator-guacamole-client/pull/183
GUACAMOLE-362: Add support for CAS ClearPass
This PR adds support to the CAS module for the ClearPass functionality, which allows CAS to pass the password back to the requesting service in an encrypted fashion, and the service will decrypt the password. The password gets assigned to the credentials for the module, so it can be used as a token.
As a side-effect, this also resolves GUACAMOLE-341 for the CAS module, which deals we the username being available in the credentials object for SSO authentication extensions.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/necouchman/incubator-guacamole-client GUACAMOLE-362
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-guacamole-client/pull/183.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #183
----
commit a7effd027b87f7f0154f51c3a4afed33c8d1cc8e
Author: Nick Couchman <vn...@apache.org>
Date: 2017-08-16T15:58:26Z
GUACAMOLE-362: Add support for CAS ClearPass, parsing and decrypting the password and assigning it a token.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142033722
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java ---
@@ -70,14 +85,93 @@ public String processUsername(String ticket) throws GuacamoleException {
try {
String confRedirectURI = confService.getRedirectURI();
Assertion a = validator.validate(ticket, confRedirectURI);
- principal = a.getPrincipal();
+ AttributePrincipal principal = a.getPrincipal();
+
+ // Retrieve username and set the credentials.
+ String username = principal.getName();
+ if (username != null)
+ credentials.setUsername(username);
+
+ // Retrieve password, attempt decryption, and set credentials.
+ Object credObj = principal.getAttributes().get("credential");
+ if (credObj != null) {
+ String clearPass = decryptPassword(credObj.toString());
+ if (clearPass != null && !clearPass.isEmpty())
+ credentials.setPassword(clearPass);
+ }
+
+ return username;
+
}
catch (TicketValidationException e) {
throw new GuacamoleException("Ticket validation failed.", e);
}
- // Return the principal name as the username.
- return principal.getName();
+ }
+
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+ private final String decryptPassword(String encryptedPassword)
+ throws GuacamoleException {
+
+ // If we get nothing, we return nothing.
+ if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+ logger.warn("No or empty encrypted password, no password will be available.");
+ return null;
+ }
+
+ final PrivateKey clearpassKey = confService.getClearpassKey();
+ if (clearpassKey == null) {
+ logger.warn("No private key available to decrypt password.");
--- End diff --
Right, I don't think authentication should fail in that case, but it seems odd that the behavior for a server with this feature enabled is to warn "No private key available to decrypt password" for every successful login.
What I'm aiming at is: shouldn't this functionality only be active if the key was provided in the first place?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r140666133
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java ---
@@ -68,4 +70,20 @@ public String getRedirectURI() throws GuacamoleException {
return environment.getRequiredProperty(CASGuacamoleProperties.CAS_REDIRECT_URI);
}
+ /**
+ * Returns the path to the file that contains the private key
+ * used to decrypt the credential that is sent encrypted by CAS,
+ * or null if no key is defined.
+ *
+ * @return
+ * The path to the private key to decrypt the ClearPass
+ * credential returned by CAS.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public Cipher getClearpassCipher() throws GuacamoleException {
+ return environment.getProperty(CASGuacamoleProperties.CAS_CLEARPASS_KEY);
+ }
+
--- End diff --
Refactored as PrivateKey.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141360569
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ final byte[] keyBytes = new byte[(int) keyFile.length()];
+ keyInput.read(keyBytes);
+ keyInput.close();
+
+ // Set up decryption infrastructure
+ KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+ KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+ return keyFactory.generatePrivate(keySpec);
+
+ }
+ catch (FileNotFoundException e) {
+ throw new GuacamoleServerException("Could not find the specified key file.", e);
+ }
+ catch (IOException e) {
+ throw new GuacamoleServerException("Could not read in the specified key file.", e);
+ }
+ catch (NoSuchAlgorithmException e) {
+ throw new GuacamoleServerException("Specified algorithm does not exist.", e);
--- End diff --
Updated.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r135426151
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java ---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements GuacamoleProperty<Cipher> {
+
+ @Override
+ public Cipher parseValue(String value) throws GuacamoleException {
+
+ try {
+
+ final Environment environment = new LocalEnvironment();
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(environment.getGuacamoleHome(), value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ final byte[] keyBytes = new byte[(int) keyFile.length()];
+ keyInput.read(keyBytes);
+ keyInput.close();
+
+ // Set up decryption infrastructure
+ KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+ KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+ final PrivateKey privateKey = keyFactory.generatePrivate(keySpec);
+ final Cipher cipher = Cipher.getInstance(privateKey.getAlgorithm());
+ cipher.init(Cipher.DECRYPT_MODE, privateKey);
+
--- End diff --
So, I'm not completely sure that this is the cleanest way to do this, or what the boundary of this property should be. I implemented it as a Cipher property, which takes in the filename of a private key relative to the guacamole home directory and generates the Cipher object out of it necessary to decrypt some string text. Thoughts on whether this is the correct level to go to, or if I should go higher-level or lower-level?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r143890461
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java ---
@@ -36,48 +47,134 @@
public class TicketValidationService {
/**
+ * Logger for this class.
+ */
+ private static final Logger logger = LoggerFactory.getLogger(TicketValidationService.class);
+
+ /**
* Service for retrieving CAS configuration information.
*/
@Inject
private ConfigurationService confService;
/**
- * Validates and parses the given ID ticket, returning the username contained
- * therein, as defined by the username claim type given in
- * guacamole.properties. If the username claim type is missing or the ID
- * ticket is invalid, an exception is thrown instead.
+ * Validates and parses the given ID ticket, returning the username
+ * provided by the CAS server in the ticket. If the
+ * ticket is invalid an exception is thrown.
*
* @param ticket
* The ID ticket to validate and parse.
*
+ * @param credentials
+ * The Credentials object to store retrieved username and
+ * password values in.
+ *
* @return
- * The username contained within the given ID ticket.
+ * The username derived from the ticket.
*
* @throws GuacamoleException
- * If the ID ticket is not valid, the username claim type is missing, or
- * guacamole.properties could not be parsed.
+ * If the ID ticket is not valid or guacamole.properties could
+ * not be parsed.
*/
- public String processUsername(String ticket) throws GuacamoleException {
-
- AttributePrincipal principal = null;
+ public String validateTicket(String ticket, Credentials credentials) throws GuacamoleException {
// Retrieve the configured CAS URL, establish a ticket validator,
// and then attempt to validate the supplied ticket. If that succeeds,
// grab the principal returned by the validator.
String casServerUrl = confService.getAuthorizationEndpoint();
Cas20ProxyTicketValidator validator = new Cas20ProxyTicketValidator(casServerUrl);
validator.setAcceptAnyProxy(true);
+ validator.setEncoding("UTF-8");
try {
String confRedirectURI = confService.getRedirectURI();
Assertion a = validator.validate(ticket, confRedirectURI);
- principal = a.getPrincipal();
+ AttributePrincipal principal = a.getPrincipal();
+
+ // Retrieve username and set the credentials.
+ String username = principal.getName();
+ if (username != null)
+ credentials.setUsername(username);
+
+ // Retrieve password, attempt decryption, and set credentials.
+ Object credObj = principal.getAttributes().get("credential");
+ if (credObj != null) {
+ String clearPass = decryptPassword(credObj.toString());
+ if (clearPass != null && !clearPass.isEmpty())
+ credentials.setPassword(clearPass);
+ }
+
+ return username;
+
}
catch (TicketValidationException e) {
throw new GuacamoleException("Ticket validation failed.", e);
}
- // Return the principal name as the username.
- return principal.getName();
+ }
+
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+ private final String decryptPassword(String encryptedPassword)
+ throws GuacamoleException {
+
+ // If we get nothing, we return nothing.
+ if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+ logger.warn("No or empty encrypted password, no password will be available.");
+ return null;
+ }
+
+ final PrivateKey clearpassKey = confService.getClearpassKey();
+ if (clearpassKey == null) {
+ logger.debug("No private key available to decrypt password.");
+ return null;
+ }
+
+ try {
+
+ final Cipher cipher = Cipher.getInstance(clearpassKey.getAlgorithm());
+
+ if (cipher == null)
+ throw new GuacamoleServerException("Failed to initialize cipher object with private key.");
+
+ // Initialize the Cipher in decrypt mode.
+ cipher.init(Cipher.DECRYPT_MODE, clearpassKey);
+
+ // Decode and decrypt, and return a new string.
+ final byte[] pass64 = DatatypeConverter.parseBase64Binary(encryptedPassword);
+ final byte[] cipherData = cipher.doFinal(pass64);
+ return new String(cipherData, Charset.forName("UTF-8"));
+
+ }
+ catch (BadPaddingException e) {
+ throw new GuacamoleServerException("Bad padding when decrypting cipher data.", e);
+ }
+ catch (IllegalBlockSizeException e) {
+ throw new GuacamoleServerException("Illegal block size while opening private key.", e);
+ }
+ catch (InvalidKeyException e) {
+ throw new GuacamoleServerException("Specified private key for ClearPass decryption is invalid.", e);
+ }
+ catch (NoSuchAlgorithmException e) {
+ throw new GuacamoleServerException("Unexpected algorithm for the private key.", e);
+ }
+ catch (NoSuchPaddingException e) {
+ throw new GuacamoleServerException("No such padding tryingto initialize cipher with private key.", e);
--- End diff --
"tryingto"
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142019920
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java ---
@@ -70,14 +85,93 @@ public String processUsername(String ticket) throws GuacamoleException {
try {
String confRedirectURI = confService.getRedirectURI();
Assertion a = validator.validate(ticket, confRedirectURI);
- principal = a.getPrincipal();
+ AttributePrincipal principal = a.getPrincipal();
+
+ // Retrieve username and set the credentials.
+ String username = principal.getName();
+ if (username != null)
+ credentials.setUsername(username);
+
+ // Retrieve password, attempt decryption, and set credentials.
+ Object credObj = principal.getAttributes().get("credential");
+ if (credObj != null) {
+ String clearPass = decryptPassword(credObj.toString());
+ if (clearPass != null && !clearPass.isEmpty())
+ credentials.setPassword(clearPass);
+ }
+
+ return username;
+
}
catch (TicketValidationException e) {
throw new GuacamoleException("Ticket validation failed.", e);
}
- // Return the principal name as the username.
- return principal.getName();
+ }
+
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+ private final String decryptPassword(String encryptedPassword)
+ throws GuacamoleException {
+
+ // If we get nothing, we return nothing.
+ if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+ logger.warn("No or empty encrypted password, no password will be available.");
+ return null;
+ }
+
+ final PrivateKey clearpassKey = confService.getClearpassKey();
+ if (clearpassKey == null) {
+ logger.warn("No private key available to decrypt password.");
--- End diff --
This will result in warnings for any system where CAS is configured to return credentials, but Guacamole is not configured to use those returned credentials. Is that the desired behavior?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134758113
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+ private final String decryptPassword(String encryptedPassword)
+ throws GuacamoleException {
+
+ // If we get nothing, we return nothing.
+ if (encryptedPassword == null || encryptedPassword.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(new LocalEnvironment().getGuacamoleHome(), confService.getClearpassKey().toString());
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ final byte[] keyBytes = new byte[(int) keyFile.length()];
+ keyInput.read(keyBytes);
+ keyInput.close();
+
+ // Set up decryption infrastructure
+ KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+ KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+ final PrivateKey privateKey = keyFactory.generatePrivate(keySpec);
+ final Cipher cipher = Cipher.getInstance(privateKey.getAlgorithm());
+ final byte[] pass64 = DatatypeConverter.parseBase64Binary(encryptedPassword);
+ cipher.init(Cipher.DECRYPT_MODE, privateKey);
+
+ // Decrypt and return a new string.
+ final byte[] cipherData = cipher.doFinal(pass64);
+ return new String(cipherData);
+ }
+ catch (FileNotFoundException e) {
+ logger.error("ClearPass key file not found, password will not be decrypted.");
+ logger.debug("Error locating the ClearPass key file: {}", e.getMessage());
--- End diff --
`logger.debug()` calls should receive the exception itself, not just the message. That way, when debug logging is enabled, full stacktraces are logged. Assuming the exception message is meaningful, it's the higher-level log statement which would receive `e.getMessage()` rather than `e` (in this case, the call to `logger.error()`).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141359733
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java ---
@@ -57,9 +57,7 @@
* If the ID ticket is not valid, the username claim type is missing, or
* guacamole.properties could not be parsed.
*/
- public String processUsername(String ticket) throws GuacamoleException {
-
- AttributePrincipal principal = null;
+ public AttributePrincipal validateTicket(String ticket) throws GuacamoleException {
--- End diff --
Updated.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142019875
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java ---
@@ -70,14 +85,93 @@ public String processUsername(String ticket) throws GuacamoleException {
try {
String confRedirectURI = confService.getRedirectURI();
Assertion a = validator.validate(ticket, confRedirectURI);
- principal = a.getPrincipal();
+ AttributePrincipal principal = a.getPrincipal();
+
+ // Retrieve username and set the credentials.
+ String username = principal.getName();
+ if (username != null)
+ credentials.setUsername(username);
+
+ // Retrieve password, attempt decryption, and set credentials.
+ Object credObj = principal.getAttributes().get("credential");
+ if (credObj != null) {
+ String clearPass = decryptPassword(credObj.toString());
+ if (clearPass != null && !clearPass.isEmpty())
+ credentials.setPassword(clearPass);
+ }
+
+ return username;
+
}
catch (TicketValidationException e) {
throw new GuacamoleException("Ticket validation failed.", e);
}
- // Return the principal name as the username.
- return principal.getName();
+ }
+
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+ private final String decryptPassword(String encryptedPassword)
+ throws GuacamoleException {
+
+ // If we get nothing, we return nothing.
+ if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+ logger.warn("No or empty encrypted password, no password will be available.");
+ return null;
+ }
+
+ final PrivateKey clearpassKey = confService.getClearpassKey();
+ if (clearpassKey == null) {
+ logger.warn("No private key available to decrypt password.");
+ return null;
+ }
+
+ try {
+
+ final Cipher cipher = Cipher.getInstance(clearpassKey.getAlgorithm());
+
+ if (cipher == null)
+ throw new GuacamoleServerException("Failed to initialize cipher object with private key.");
+
+ // Initialize the Cipher in decrypt mode.
+ cipher.init(Cipher.DECRYPT_MODE, clearpassKey);
+
+ // Decode and decrypt, and return a new string.
+ final byte[] pass64 = DatatypeConverter.parseBase64Binary(encryptedPassword);
+ final byte[] cipherData = cipher.doFinal(pass64);
+ return new String(cipherData);
--- End diff --
The `String` constructor which takes only a `byte[]` uses the default charset, and is generally considered unsafe for production code. The encoding used by the data within the `byte[]` should be known.
Does CAS define this?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r140666358
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java ---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements GuacamoleProperty<Cipher> {
+
+ @Override
+ public Cipher parseValue(String value) throws GuacamoleException {
+
+ try {
+
+ final Environment environment = new LocalEnvironment();
+
--- End diff --
Pulled out home env call and we'll just require absolute path.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r140645395
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java ---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements GuacamoleProperty<Cipher> {
+
+ @Override
+ public Cipher parseValue(String value) throws GuacamoleException {
+
+ try {
+
+ final Environment environment = new LocalEnvironment();
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(environment.getGuacamoleHome(), value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ final byte[] keyBytes = new byte[(int) keyFile.length()];
+ keyInput.read(keyBytes);
+ keyInput.close();
+
+ // Set up decryption infrastructure
+ KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+ KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+ final PrivateKey privateKey = keyFactory.generatePrivate(keySpec);
+ final Cipher cipher = Cipher.getInstance(privateKey.getAlgorithm());
+ cipher.init(Cipher.DECRYPT_MODE, privateKey);
+
+ return cipher;
+
+ }
+ catch (FileNotFoundException e) {
+ throw new GuacamoleException("Could not find the specified key file.", e);
+ }
+ catch (IOException e) {
+ throw new GuacamoleException("Could not read in the specified key file.", e);
+ }
+ catch (NoSuchAlgorithmException e) {
+ throw new GuacamoleException("Specified algorithm does not exist.", e);
+ }
+ catch (InvalidKeyException e) {
+ throw new GuacamoleException("Specified key is invalid.", e);
+ }
+ catch (InvalidKeySpecException e) {
+ throw new GuacamoleException("Invalid KeySpec initialization.", e);
+ }
+ catch (NoSuchPaddingException e) {
+ throw new GuacamoleException("No such padding exception.", e);
+ }
+
--- End diff --
> Is it expected that this will occasionally fail under normal (not misconfigured) conditions?
The more I think about it, the more I think this probably would not fail under anything but a misconfiguration. In order to get ClearPass to work in CAS you have to generate public and private keys, and configure a service in CAS specifically to support the encryption of the stored password, so anyone who is setting this up also has to be setting up the CAS side of things, and if they get it wrong, well, it probably shouldn't work at all. I'll change the exception to GuacamoleServerException and stick with failing authentication if this fails.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r139978510
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java ---
@@ -68,4 +70,20 @@ public String getRedirectURI() throws GuacamoleException {
return environment.getRequiredProperty(CASGuacamoleProperties.CAS_REDIRECT_URI);
}
+ /**
+ * Returns the path to the file that contains the private key
+ * used to decrypt the credential that is sent encrypted by CAS,
+ * or null if no key is defined.
+ *
+ * @return
+ * The path to the private key to decrypt the ClearPass
+ * credential returned by CAS.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public Cipher getClearpassCipher() throws GuacamoleException {
+ return environment.getProperty(CASGuacamoleProperties.CAS_CLEARPASS_KEY);
+ }
+
--- End diff --
I would tend to agree there. Why not `PrivateKey`?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r135426332
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java ---
@@ -68,4 +70,20 @@ public String getRedirectURI() throws GuacamoleException {
return environment.getRequiredProperty(CASGuacamoleProperties.CAS_REDIRECT_URI);
}
+ /**
+ * Returns the path to the file that contains the private key
+ * used to decrypt the credential that is sent encrypted by CAS,
+ * or null if no key is defined.
+ *
+ * @return
+ * The path to the private key to decrypt the ClearPass
+ * credential returned by CAS.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public Cipher getClearpassCipher() throws GuacamoleException {
+ return environment.getProperty(CASGuacamoleProperties.CAS_CLEARPASS_KEY);
+ }
+
--- End diff --
Taking a key argument and calling this a cipher property feels a little kludgy. Kind of goes along with the question about how far the Cipher property should go or if that's even really a good name/function for it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134759279
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+ private final String decryptPassword(String encryptedPassword)
+ throws GuacamoleException {
+
+ // If we get nothing, we return nothing.
+ if (encryptedPassword == null || encryptedPassword.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(new LocalEnvironment().getGuacamoleHome(), confService.getClearpassKey().toString());
--- End diff --
Is an `Environment` instance already created and available? Rather than creating a new `LocalEnvironment` each time an attempt to decrypt the password is made, it would be better to share the existing `Environment` used by the rest of the extension.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142038882
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java ---
@@ -70,14 +85,93 @@ public String processUsername(String ticket) throws GuacamoleException {
try {
String confRedirectURI = confService.getRedirectURI();
Assertion a = validator.validate(ticket, confRedirectURI);
- principal = a.getPrincipal();
+ AttributePrincipal principal = a.getPrincipal();
+
+ // Retrieve username and set the credentials.
+ String username = principal.getName();
+ if (username != null)
+ credentials.setUsername(username);
+
+ // Retrieve password, attempt decryption, and set credentials.
+ Object credObj = principal.getAttributes().get("credential");
+ if (credObj != null) {
+ String clearPass = decryptPassword(credObj.toString());
+ if (clearPass != null && !clearPass.isEmpty())
+ credentials.setPassword(clearPass);
+ }
+
+ return username;
+
}
catch (TicketValidationException e) {
throw new GuacamoleException("Ticket validation failed.", e);
}
- // Return the principal name as the username.
- return principal.getName();
+ }
+
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+ private final String decryptPassword(String encryptedPassword)
+ throws GuacamoleException {
+
+ // If we get nothing, we return nothing.
+ if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+ logger.warn("No or empty encrypted password, no password will be available.");
+ return null;
+ }
+
+ final PrivateKey clearpassKey = confService.getClearpassKey();
+ if (clearpassKey == null) {
+ logger.warn("No private key available to decrypt password.");
--- End diff --
I changed the warn to debug. My thought process is that, if you are setting up both CAS and Guacamole and you're looking for hints as to why it isn't working, you want a message that indicates that the private key isn't loading. I don't know that the order of these first two statements makes all that much difference - I can have the check for private key come, first, if that's more desirable, but either way it needs to check for both the presence of the encrypted password and the private key before continuing.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r143899357
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ int keyLength = (int) keyFile.length();
+ final byte[] keyBytes = new byte[keyLength];
--- End diff --
Okay, took a stab at a loop implementation of this...
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142019203
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java ---
@@ -57,21 +57,21 @@
private ConfigurationService confService;
/**
- * Validates and parses the given ID ticket, returning the Credentials object
- * derived from the parameters provided by the CAS server in the ticket. If the
+ * Validates and parses the given ID ticket, returning the username
+ * provided by the CAS server in the ticket. If the
* ticket is invalid an exception is thrown.
*
* @param ticket
* The ID ticket to validate and parse.
*
* @return
- * The Credentials object derived from parameters provided in the ticket.
+ * The username derived from the ticket.
*
* @throws GuacamoleException
* If the ID ticket is not valid or guacamole.properties could
* not be parsed.
*/
- public Credentials validateTicket(String ticket) throws GuacamoleException {
+ public String validateTicket(String ticket, Credentials credentials) throws GuacamoleException {
--- End diff --
Oops...added.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134765905
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+ private final String decryptPassword(String encryptedPassword)
+ throws GuacamoleException {
+
+ // If we get nothing, we return nothing.
+ if (encryptedPassword == null || encryptedPassword.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(new LocalEnvironment().getGuacamoleHome(), confService.getClearpassKey().toString());
--- End diff --
Should be fixed, though it won't matter if this gets pulled out into its own property type.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134763447
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -83,7 +106,16 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
String ticket = request.getParameter(CASTicketField.PARAMETER_NAME);
if (ticket != null) {
AuthenticatedUser authenticatedUser = authenticatedUserProvider.get();
- authenticatedUser.init(ticketService.processUsername(ticket), credentials);
+ AttributePrincipal principal = ticketService.validateTicket(ticket);
+ String username = principal.getName();
+ credentials.setUsername(username);
+ Object credObj = principal.getAttributes().get("credential");
+ if (credObj != null) {
+ String clearPass = decryptPassword(credObj.toString());
--- End diff --
The getAttributes call returns a Map<String, Object>, so I think this conversion is the only (safe) way to get this object into the string format needed for the decryption process.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r143890893
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ int keyLength = (int) keyFile.length();
+ final byte[] keyBytes = new byte[keyLength];
+ int keyRead = keyInput.read(keyBytes);
+
+ // Error reading any bytes out of the key.
+ if (keyRead == -1)
+ throw new GuacamoleServerException("Failed to get any bytes while reading key.");
+
+ // Zero-sized key
+ else if(keyRead == 0)
--- End diff --
A `read()` of 0 does not indicate that there are zero bytes in the file, nor any error condition. It simply means that `read()` read nothing, and should be invoked again.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134767849
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+ private final String decryptPassword(String encryptedPassword)
+ throws GuacamoleException {
+
+ // If we get nothing, we return nothing.
+ if (encryptedPassword == null || encryptedPassword.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(new LocalEnvironment().getGuacamoleHome(), confService.getClearpassKey().toString());
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ final byte[] keyBytes = new byte[(int) keyFile.length()];
+ keyInput.read(keyBytes);
+ keyInput.close();
+
+ // Set up decryption infrastructure
+ KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+ KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+ final PrivateKey privateKey = keyFactory.generatePrivate(keySpec);
+ final Cipher cipher = Cipher.getInstance(privateKey.getAlgorithm());
+ final byte[] pass64 = DatatypeConverter.parseBase64Binary(encryptedPassword);
+ cipher.init(Cipher.DECRYPT_MODE, privateKey);
+
+ // Decrypt and return a new string.
+ final byte[] cipherData = cipher.doFinal(pass64);
+ return new String(cipherData);
+ }
+ catch (FileNotFoundException e) {
+ logger.error("ClearPass key file not found, password will not be decrypted.");
+ logger.debug("Error locating the ClearPass key file: {}", e.getMessage());
--- End diff --
This should be fixed - I don't know that any of the error messages are worth passing to logger.error(), so I just those as-is, and changed logger.debug() to include the full exception.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r140689214
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -105,4 +146,59 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+ private final String decryptPassword(String encryptedPassword)
+ throws GuacamoleException {
+
+ // If we get nothing, we return nothing.
+ if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+ logger.warn("No or empty encrypted password, no password will be available.");
+ return null;
+ }
+
+ final PrivateKey clearpassKey = confService.getClearpassKey();
+ if (clearpassKey == null) {
+ logger.warn("No private key available to decrypt password.");
+ return null;
+ }
+
+ try {
+
+ final Cipher cipher = Cipher.getInstance(clearpassKey.getAlgorithm());
+
+ if (cipher == null)
+ throw new GuacamoleServerException("Failed to initialize cipher object with private key.");
+
+ // Initialize the Cipher in decrypt mode.
+ cipher.init(Cipher.DECRYPT_MODE, clearpassKey);
+
+ // Decode and decrypt, and return a new string.
+ final byte[] pass64 = DatatypeConverter.parseBase64Binary(encryptedPassword);
+ final byte[] cipherData = cipher.doFinal(pass64);
+ return new String(cipherData);
+
+ }
+ catch (Throwable t) {
--- End diff --
Why are we catching `Throwable`?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r135920241
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java ---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements GuacamoleProperty<Cipher> {
+
+ @Override
+ public Cipher parseValue(String value) throws GuacamoleException {
+
+ try {
+
+ final Environment environment = new LocalEnvironment();
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(environment.getGuacamoleHome(), value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ final byte[] keyBytes = new byte[(int) keyFile.length()];
+ keyInput.read(keyBytes);
+ keyInput.close();
+
+ // Set up decryption infrastructure
+ KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+ KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+ final PrivateKey privateKey = keyFactory.generatePrivate(keySpec);
+ final Cipher cipher = Cipher.getInstance(privateKey.getAlgorithm());
+ cipher.init(Cipher.DECRYPT_MODE, privateKey);
+
+ return cipher;
+
+ }
+ catch (FileNotFoundException e) {
+ throw new GuacamoleException("Could not find the specified key file.", e);
+ }
+ catch (IOException e) {
+ throw new GuacamoleException("Could not read in the specified key file.", e);
+ }
+ catch (NoSuchAlgorithmException e) {
+ throw new GuacamoleException("Specified algorithm does not exist.", e);
+ }
+ catch (InvalidKeyException e) {
+ throw new GuacamoleException("Specified key is invalid.", e);
+ }
+ catch (InvalidKeySpecException e) {
+ throw new GuacamoleException("Invalid KeySpec initialization.", e);
+ }
+ catch (NoSuchPaddingException e) {
+ throw new GuacamoleException("No such padding exception.", e);
+ }
+
--- End diff --
What do you mean by "killed that off"?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r140666346
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java ---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements GuacamoleProperty<Cipher> {
+
+ @Override
+ public Cipher parseValue(String value) throws GuacamoleException {
+
+ try {
+
+ final Environment environment = new LocalEnvironment();
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(environment.getGuacamoleHome(), value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ final byte[] keyBytes = new byte[(int) keyFile.length()];
+ keyInput.read(keyBytes);
+ keyInput.close();
+
+ // Set up decryption infrastructure
+ KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+ KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+ final PrivateKey privateKey = keyFactory.generatePrivate(keySpec);
+ final Cipher cipher = Cipher.getInstance(privateKey.getAlgorithm());
+ cipher.init(Cipher.DECRYPT_MODE, privateKey);
+
+ return cipher;
+
+ }
+ catch (FileNotFoundException e) {
+ throw new GuacamoleException("Could not find the specified key file.", e);
+ }
+ catch (IOException e) {
+ throw new GuacamoleException("Could not read in the specified key file.", e);
+ }
+ catch (NoSuchAlgorithmException e) {
+ throw new GuacamoleException("Specified algorithm does not exist.", e);
+ }
+ catch (InvalidKeyException e) {
+ throw new GuacamoleException("Specified key is invalid.", e);
+ }
+ catch (InvalidKeySpecException e) {
+ throw new GuacamoleException("Invalid KeySpec initialization.", e);
+ }
+ catch (NoSuchPaddingException e) {
+ throw new GuacamoleException("No such padding exception.", e);
+ }
+
--- End diff --
So, I've redone most of this such that it throws the GuacamoleServerException. There are two scenarios I can think of where having authentication succeed despite some error in the ClearPass decryption process would be desirable:
- If the credentials object is provided by the CAS server, but the Guacamole admin has not configured a private key, I think authentication should still succeed. Since, in many organizations, SSO is run by someone different than a VDI/Desktop/RemoteAccess person, it's conceivable that the CAS server may provide something we choose not to consume, and that should not cause an error.
- Where the Guacamole admin has configured a PrivateKey, but CAS is not providing a value for the credential parameter. Again, with the potential for CAS and Guacamole to be run by different admins/groups, or for different users within CAS to have different policies applied, it's conceivable that the GuacamoleAdmin configures a PrivateKey file for this purpose, but the attribute is blank/null.
Is my logic sound there?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r147463115
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ int keyLength = (int) keyFile.length();
+ final byte[] keyBytes = new byte[keyLength];
--- End diff --
Okay, so in the interest of getting this prepped more quickly for the 0.9.14-incubating release, I've moved the property to the CAS extension, and also implemented the the `ByteArrayOutputStream` solution instead of pulling in the extra package and changing the version requirement.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141265939
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java ---
@@ -57,9 +57,7 @@
* If the ID ticket is not valid, the username claim type is missing, or
* guacamole.properties could not be parsed.
*/
- public String processUsername(String ticket) throws GuacamoleException {
-
- AttributePrincipal principal = null;
+ public AttributePrincipal validateTicket(String ticket) throws GuacamoleException {
--- End diff --
The documentation for this function states:
> Validates and parses the given ID ticket, returning the username contained therein, ...
This was from when the function returned a `String` and was called `processUsername()`. Now that the function has been reworked to return an `AttributePrincipal` (and renamed to `validateTicket()`), is this documentation still valid?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r135947971
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java ---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements GuacamoleProperty<Cipher> {
+
+ @Override
+ public Cipher parseValue(String value) throws GuacamoleException {
+
+ try {
+
+ final Environment environment = new LocalEnvironment();
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(environment.getGuacamoleHome(), value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ final byte[] keyBytes = new byte[(int) keyFile.length()];
+ keyInput.read(keyBytes);
+ keyInput.close();
+
+ // Set up decryption infrastructure
+ KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+ KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+ final PrivateKey privateKey = keyFactory.generatePrivate(keySpec);
+ final Cipher cipher = Cipher.getInstance(privateKey.getAlgorithm());
+ cipher.init(Cipher.DECRYPT_MODE, privateKey);
+
+ return cipher;
+
+ }
+ catch (FileNotFoundException e) {
+ throw new GuacamoleException("Could not find the specified key file.", e);
+ }
+ catch (IOException e) {
+ throw new GuacamoleException("Could not read in the specified key file.", e);
+ }
+ catch (NoSuchAlgorithmException e) {
+ throw new GuacamoleException("Specified algorithm does not exist.", e);
+ }
+ catch (InvalidKeyException e) {
+ throw new GuacamoleException("Specified key is invalid.", e);
+ }
+ catch (InvalidKeySpecException e) {
+ throw new GuacamoleException("Invalid KeySpec initialization.", e);
+ }
+ catch (NoSuchPaddingException e) {
+ throw new GuacamoleException("No such padding exception.", e);
+ }
+
--- End diff --
When all of those exceptions were caught and dealt with directly in the AuthenticationProviderService class, the mehtod just returned null rather than throwing a GuacamoleException. Returning null just means that the password is not added to the credential object, so it isn't available as a token later on inside the client.
Since creating a more generic type, here, I figured, while the behavior of just returning a null value and not killing authentication entirely is fine for the CAS extension, it may not be desirable across the board for whatever stuff may come later that might use this property, so instead of returning null, here, I'm throwing GuacamoleException. This means that, if those exceptions are thrown while CAS is using this property, it'll kill CAS authentication if this Cipher property fails to parse for one of these reasons. I consider that behavior less desirable than just not having the password available to the user, as authentication has technically still succeeded.
I suppose I could capture the GuacamoleException inside the CAS module when pulling this property and just return null instead of allowing the GuacamoleException to pass through...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r135426231
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java ---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements GuacamoleProperty<Cipher> {
+
+ @Override
+ public Cipher parseValue(String value) throws GuacamoleException {
+
+ try {
+
+ final Environment environment = new LocalEnvironment();
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(environment.getGuacamoleHome(), value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ final byte[] keyBytes = new byte[(int) keyFile.length()];
+ keyInput.read(keyBytes);
+ keyInput.close();
+
+ // Set up decryption infrastructure
+ KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+ KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+ final PrivateKey privateKey = keyFactory.generatePrivate(keySpec);
+ final Cipher cipher = Cipher.getInstance(privateKey.getAlgorithm());
+ cipher.init(Cipher.DECRYPT_MODE, privateKey);
+
+ return cipher;
+
+ }
+ catch (FileNotFoundException e) {
+ throw new GuacamoleException("Could not find the specified key file.", e);
+ }
+ catch (IOException e) {
+ throw new GuacamoleException("Could not read in the specified key file.", e);
+ }
+ catch (NoSuchAlgorithmException e) {
+ throw new GuacamoleException("Specified algorithm does not exist.", e);
+ }
+ catch (InvalidKeyException e) {
+ throw new GuacamoleException("Specified key is invalid.", e);
+ }
+ catch (InvalidKeySpecException e) {
+ throw new GuacamoleException("Invalid KeySpec initialization.", e);
+ }
+ catch (NoSuchPaddingException e) {
+ throw new GuacamoleException("No such padding exception.", e);
+ }
+
--- End diff --
And, you know how I said that the exceptions shouldn't kill the entire client, it should just result in the password not being available...well...I think all of this here killed that off...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r140645375
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java ---
@@ -68,4 +70,20 @@ public String getRedirectURI() throws GuacamoleException {
return environment.getRequiredProperty(CASGuacamoleProperties.CAS_REDIRECT_URI);
}
+ /**
+ * Returns the path to the file that contains the private key
+ * used to decrypt the credential that is sent encrypted by CAS,
+ * or null if no key is defined.
+ *
+ * @return
+ * The path to the private key to decrypt the ClearPass
+ * credential returned by CAS.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public Cipher getClearpassCipher() throws GuacamoleException {
+ return environment.getProperty(CASGuacamoleProperties.CAS_CLEARPASS_KEY);
+ }
+
--- End diff --
Yeah, PrivateKey is probably a better route to go, and just keep the Cipher pieces of it in the CAS extension code. I'll try to rework this a little bit.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141361436
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -83,7 +115,15 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
String ticket = request.getParameter(CASTicketField.PARAMETER_NAME);
if (ticket != null) {
AuthenticatedUser authenticatedUser = authenticatedUserProvider.get();
- authenticatedUser.init(ticketService.processUsername(ticket), credentials);
+ AttributePrincipal principal = ticketService.validateTicket(ticket);
+ String username = principal.getName();
+ Object credObj = principal.getAttributes().get("credential");
+ if (credObj != null) {
--- End diff --
Let me see what I can figure out. That's essentially what it was before, and only the username was returned. When this transitioned to also providing a password I re-arranged things so that it would be a little more straight-forward to retrieve both the username and the password from the AttributePrincipal object without having to try to pass a HashMap or something like that. If you have a direction in mind let me know...otherwise I'll just take a stab at something and see what I can figure out.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r144685933
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ int keyLength = (int) keyFile.length();
+ final byte[] keyBytes = new byte[keyLength];
--- End diff --
> ... any reason not to use Apache Commons IO?
If you want to use Apache Commons IO, I'm not against it.
To me, the only reason would be to avoid bundling a library from which we're only going to use one function. We've removed dependencies in the past for similar reasons (see #37), but in that case there was a reasonable alternative already available within Java itself. If the alternative in the case of #37 was to write our own implementation entirely, I'm sure we would have kept the dependency around.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141360655
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java ---
@@ -68,4 +70,20 @@ public String getRedirectURI() throws GuacamoleException {
return environment.getRequiredProperty(CASGuacamoleProperties.CAS_REDIRECT_URI);
}
+ /**
+ * Returns the path to the file that contains the private key
--- End diff --
Updated.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142019777
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ final byte[] keyBytes = new byte[(int) keyFile.length()];
+ keyInput.read(keyBytes);
+ keyInput.close();
+
+ // Set up decryption infrastructure
+ KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+ KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+ return keyFactory.generatePrivate(keySpec);
+
+ }
+ catch (FileNotFoundException e) {
+ throw new GuacamoleServerException("Could not find the specified key file.", e);
+ }
+ catch (IOException e) {
+ throw new GuacamoleServerException("Could not read in the specified key file.", e);
+ }
+ catch (NoSuchAlgorithmException e) {
+ throw new GuacamoleServerException("Key is not in expected RSA algorithm.", e);
+ }
+ catch (InvalidKeySpecException e) {
+ throw new GuacamoleServerException("KeyS is not in expected PKCS8 encoding.", e);
--- End diff --
"KeyS"
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142018263
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -82,8 +87,17 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
if (request != null) {
String ticket = request.getParameter(CASTicketField.PARAMETER_NAME);
if (ticket != null) {
+ Credentials ticketCredentials = ticketService.validateTicket(ticket);
+ if (ticketCredentials != null) {
+ String username = ticketCredentials.getUsername();
+ if (username != null)
+ credentials.setUsername(username);
+ String password = ticketCredentials.getPassword();
+ if (password != null)
+ credentials.setPassword(password);
+ }
AuthenticatedUser authenticatedUser = authenticatedUserProvider.get();
- authenticatedUser.init(ticketService.processUsername(ticket), credentials);
+ authenticatedUser.init(credentials.getUsername(), credentials);
--- End diff --
Okay, I refactored as you suggested, and I believe I have all of the username objects coming from CAS.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r143890955
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ int keyLength = (int) keyFile.length();
+ final byte[] keyBytes = new byte[keyLength];
+ int keyRead = keyInput.read(keyBytes);
+
+ // Error reading any bytes out of the key.
+ if (keyRead == -1)
--- End diff --
Normally, this is the condition you would check to determine whether to stop reading the file due to reaching EOF.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142019841
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ final byte[] keyBytes = new byte[(int) keyFile.length()];
+ keyInput.read(keyBytes);
+ keyInput.close();
+
+ // Set up decryption infrastructure
+ KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+ KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+ return keyFactory.generatePrivate(keySpec);
+
+ }
+ catch (FileNotFoundException e) {
+ throw new GuacamoleServerException("Could not find the specified key file.", e);
+ }
+ catch (IOException e) {
+ throw new GuacamoleServerException("Could not read in the specified key file.", e);
+ }
+ catch (NoSuchAlgorithmException e) {
+ throw new GuacamoleServerException("Key is not in expected RSA algorithm.", e);
--- End diff --
The `NoSuchAlgorithmException` does not indicate that the key is not a valid RSA key, but rather that the RSA algorithm isn't available (ie: the JVM in use doesn't provide it).
https://docs.oracle.com/javase/8/docs/api/java/security/NoSuchAlgorithmException.html
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r140944932
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -105,4 +146,59 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+ private final String decryptPassword(String encryptedPassword)
+ throws GuacamoleException {
+
+ // If we get nothing, we return nothing.
+ if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+ logger.warn("No or empty encrypted password, no password will be available.");
+ return null;
+ }
+
+ final PrivateKey clearpassKey = confService.getClearpassKey();
+ if (clearpassKey == null) {
+ logger.warn("No private key available to decrypt password.");
+ return null;
+ }
+
+ try {
+
+ final Cipher cipher = Cipher.getInstance(clearpassKey.getAlgorithm());
+
+ if (cipher == null)
+ throw new GuacamoleServerException("Failed to initialize cipher object with private key.");
+
+ // Initialize the Cipher in decrypt mode.
+ cipher.init(Cipher.DECRYPT_MODE, clearpassKey);
+
+ // Decode and decrypt, and return a new string.
+ final byte[] pass64 = DatatypeConverter.parseBase64Binary(encryptedPassword);
+ final byte[] cipherData = cipher.doFinal(pass64);
+ return new String(cipherData);
+
+ }
+ catch (Throwable t) {
--- End diff --
I think the number of things I was catching individually was getting really long - after I hit 8, I think, I decided maybe just catching Throwable was easier than going through and catching them each individually.
Is catching Throwable considered poor form (lazy, perhaps :-) vs. catching each item individually, particularly if they're all going to be re-thrown as GuacamoleServerExceptions, anyway? I'll go back the other way if that's the proper way to do it...
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r144460945
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ int keyLength = (int) keyFile.length();
+ final byte[] keyBytes = new byte[keyLength];
--- End diff --
Looks better, however:
1. The duplicate `read()` calls are somewhat odd.
2. This loop implementation will spin indefinitely. When the buffer is filled, `keyBytes.length - totalBytesRead` becomes 0, in which case the call to `read()` will be asked to read 0 bytes and will always return 0, never signalling EOF.
It might be a lot easier to implement correctly if the byte array is allocated dynamically. That would also allow for cases where the file size is not known ahead of time, and remove the need to handle unexpected changes in file size.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-guacamole-client/pull/183
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r144681030
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ int keyLength = (int) keyFile.length();
+ final byte[] keyBytes = new byte[keyLength];
--- End diff --
Well, I think I have a reasonable ByteArrayOutputStream solution, but, before I do that, any reason not to use Apache Commons IO?
https://commons.apache.org/proper/commons-io/javadocs/api-2.5/org/apache/commons/io/FileUtils.html#readFileToByteArray(java.io.File)
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134761374
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java ---
@@ -68,4 +69,20 @@ public String getRedirectURI() throws GuacamoleException {
return environment.getRequiredProperty(CASGuacamoleProperties.CAS_REDIRECT_URI);
}
+ /**
+ * Returns the path to the file that contains the private key
+ * used to decrypt the credential that is sent encrypted by CAS,
+ * or null if no key is defined.
+ *
+ * @return
+ * The path to the private key to decrypt the ClearPass
+ * credential returned by CAS.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public File getClearpassKey() throws GuacamoleException {
--- End diff --
That's probably reasonable - I'll work on moving this over to some sort of EncryptedGuacamoleProperty or something like that. Preferences on naming??
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r139979934
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java ---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements GuacamoleProperty<Cipher> {
+
+ @Override
+ public Cipher parseValue(String value) throws GuacamoleException {
+
+ try {
+
+ final Environment environment = new LocalEnvironment();
+
--- End diff --
Hm ... I see what you mean.
It would be possible to get Guice to inject the `Environment` by using a `Provider<CipherGuacamoleProperty>`, thus avoiding recreating things, but perhaps there is a different way.
This may end up reverting things back to how you had them, but what about relying on the value of this property to be the absolute path to the key? If the property itself ends up needing to be simply a `File` or `String`, with the actual reading into `PrivateKey` and finally `Cipher` being abstracted only within the configuration service, that's fine, I'm sure.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r144708371
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ int keyLength = (int) keyFile.length();
+ final byte[] keyBytes = new byte[keyLength];
--- End diff --
Java 7 has a `readAllBytes` method in its `java.nio.file.Files` class:
http://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#readAllBytes(java.nio.file.Path)
However, I thought I had seen some things in the pom.xml files that indicated that we try to maintain Java 6 compatibility, so I avoided that route. If I'm mistaken about that or we're willing to put a Java 7 requirement on it, we could use the built-in and avoid both writing our own and pulling in a bundled library for a single method.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141360602
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ final byte[] keyBytes = new byte[(int) keyFile.length()];
+ keyInput.read(keyBytes);
+ keyInput.close();
+
+ // Set up decryption infrastructure
+ KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+ KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+ return keyFactory.generatePrivate(keySpec);
+
+ }
+ catch (FileNotFoundException e) {
+ throw new GuacamoleServerException("Could not find the specified key file.", e);
+ }
+ catch (IOException e) {
+ throw new GuacamoleServerException("Could not read in the specified key file.", e);
+ }
+ catch (NoSuchAlgorithmException e) {
+ throw new GuacamoleServerException("Specified algorithm does not exist.", e);
+ }
+ catch (InvalidKeySpecException e) {
+ throw new GuacamoleServerException("Invalid KeySpec initialization.", e);
--- End diff --
Updated.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142027015
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java ---
@@ -70,14 +85,93 @@ public String processUsername(String ticket) throws GuacamoleException {
try {
String confRedirectURI = confService.getRedirectURI();
Assertion a = validator.validate(ticket, confRedirectURI);
- principal = a.getPrincipal();
+ AttributePrincipal principal = a.getPrincipal();
+
+ // Retrieve username and set the credentials.
+ String username = principal.getName();
+ if (username != null)
+ credentials.setUsername(username);
+
+ // Retrieve password, attempt decryption, and set credentials.
+ Object credObj = principal.getAttributes().get("credential");
+ if (credObj != null) {
+ String clearPass = decryptPassword(credObj.toString());
+ if (clearPass != null && !clearPass.isEmpty())
+ credentials.setPassword(clearPass);
+ }
+
+ return username;
+
}
catch (TicketValidationException e) {
throw new GuacamoleException("Ticket validation failed.", e);
}
- // Return the principal name as the username.
- return principal.getName();
+ }
+
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+ private final String decryptPassword(String encryptedPassword)
+ throws GuacamoleException {
+
+ // If we get nothing, we return nothing.
+ if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+ logger.warn("No or empty encrypted password, no password will be available.");
+ return null;
+ }
+
+ final PrivateKey clearpassKey = confService.getClearpassKey();
+ if (clearpassKey == null) {
+ logger.warn("No private key available to decrypt password.");
--- End diff --
Yes, that is desired - since there are situations where it is conceivable that the Guacamole administrator does not have control over the CAS server and what attributes are returned by the server, it would be undesirable to fail authentication completely just because CAS returned attributes that Guacamole was not expecting. While that's unlikely in the case of ClearPass, I still believe it is the correct behavior.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134755694
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java ---
@@ -68,4 +69,20 @@ public String getRedirectURI() throws GuacamoleException {
return environment.getRequiredProperty(CASGuacamoleProperties.CAS_REDIRECT_URI);
}
+ /**
+ * Returns the path to the file that contains the private key
+ * used to decrypt the credential that is sent encrypted by CAS,
+ * or null if no key is defined.
+ *
+ * @return
+ * The path to the private key to decrypt the ClearPass
+ * credential returned by CAS.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public File getClearpassKey() throws GuacamoleException {
--- End diff --
Thoughts on implementing a `GuacamoleProperty` subclass which handles the parsing of the key value, thus abstracting away the reading of its contents as `KeySpec` or `PrivateKey` or similar?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141266280
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ final byte[] keyBytes = new byte[(int) keyFile.length()];
+ keyInput.read(keyBytes);
+ keyInput.close();
+
+ // Set up decryption infrastructure
+ KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+ KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+ return keyFactory.generatePrivate(keySpec);
+
+ }
+ catch (FileNotFoundException e) {
+ throw new GuacamoleServerException("Could not find the specified key file.", e);
+ }
+ catch (IOException e) {
+ throw new GuacamoleServerException("Could not read in the specified key file.", e);
+ }
+ catch (NoSuchAlgorithmException e) {
+ throw new GuacamoleServerException("Specified algorithm does not exist.", e);
--- End diff --
Since we're the ones specifying the algorithm, we should probably specifically state the cause of the error in this case, as it is known.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142027025
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java ---
@@ -70,14 +85,93 @@ public String processUsername(String ticket) throws GuacamoleException {
try {
String confRedirectURI = confService.getRedirectURI();
Assertion a = validator.validate(ticket, confRedirectURI);
- principal = a.getPrincipal();
+ AttributePrincipal principal = a.getPrincipal();
+
+ // Retrieve username and set the credentials.
+ String username = principal.getName();
+ if (username != null)
+ credentials.setUsername(username);
+
+ // Retrieve password, attempt decryption, and set credentials.
+ Object credObj = principal.getAttributes().get("credential");
+ if (credObj != null) {
+ String clearPass = decryptPassword(credObj.toString());
+ if (clearPass != null && !clearPass.isEmpty())
+ credentials.setPassword(clearPass);
+ }
+
+ return username;
+
}
catch (TicketValidationException e) {
throw new GuacamoleException("Ticket validation failed.", e);
}
- // Return the principal name as the username.
- return principal.getName();
+ }
+
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+ private final String decryptPassword(String encryptedPassword)
+ throws GuacamoleException {
+
+ // If we get nothing, we return nothing.
+ if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+ logger.warn("No or empty encrypted password, no password will be available.");
+ return null;
+ }
+
+ final PrivateKey clearpassKey = confService.getClearpassKey();
+ if (clearpassKey == null) {
+ logger.warn("No private key available to decrypt password.");
+ return null;
+ }
+
+ try {
+
+ final Cipher cipher = Cipher.getInstance(clearpassKey.getAlgorithm());
+
+ if (cipher == null)
+ throw new GuacamoleServerException("Failed to initialize cipher object with private key.");
+
+ // Initialize the Cipher in decrypt mode.
+ cipher.init(Cipher.DECRYPT_MODE, clearpassKey);
+
+ // Decode and decrypt, and return a new string.
+ final byte[] pass64 = DatatypeConverter.parseBase64Binary(encryptedPassword);
+ final byte[] cipherData = cipher.doFinal(pass64);
+ return new String(cipherData);
--- End diff --
I'll have to dig into the CAS side and see.
Is there a preferred way to convert byte[] data to a String in Java that is considered production-safe?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r144711034
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ int keyLength = (int) keyFile.length();
+ final byte[] keyBytes = new byte[keyLength];
--- End diff --
While stopping support for Java 6 entirely would need some discussion, requiring Java 7 for a particular extension (such as the CAS auth) seems OK from my perspective.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134754446
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java ---
@@ -57,9 +57,7 @@
* If the ID ticket is not valid, the username claim type is missing, or
* guacamole.properties could not be parsed.
*/
- public String processUsername(String ticket) throws GuacamoleException {
-
- AttributePrincipal principal = null;
--- End diff --
Glad to see this line go. ;) Never a fan of unnecessary initialization.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134756239
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+ private final String decryptPassword(String encryptedPassword)
+ throws GuacamoleException {
+
+ // If we get nothing, we return nothing.
+ if (encryptedPassword == null || encryptedPassword.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(new LocalEnvironment().getGuacamoleHome(), confService.getClearpassKey().toString());
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ final byte[] keyBytes = new byte[(int) keyFile.length()];
+ keyInput.read(keyBytes);
+ keyInput.close();
+
+ // Set up decryption infrastructure
+ KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+ KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+ final PrivateKey privateKey = keyFactory.generatePrivate(keySpec);
+ final Cipher cipher = Cipher.getInstance(privateKey.getAlgorithm());
+ final byte[] pass64 = DatatypeConverter.parseBase64Binary(encryptedPassword);
--- End diff --
Beware that this will throw an `IllegalArgumentException` provided data is not valid base64. That case will need to be handled for this function to behave in a robust manner.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134774695
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java ---
@@ -68,4 +69,20 @@ public String getRedirectURI() throws GuacamoleException {
return environment.getRequiredProperty(CASGuacamoleProperties.CAS_REDIRECT_URI);
}
+ /**
+ * Returns the path to the file that contains the private key
+ * used to decrypt the credential that is sent encrypted by CAS,
+ * or null if no key is defined.
+ *
+ * @return
+ * The path to the private key to decrypt the ClearPass
+ * credential returned by CAS.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public File getClearpassKey() throws GuacamoleException {
--- End diff --
Oops...I was thinking the wrong way, here. So, it wouldn't be EncryptedGuacamoleProperty, it would be something like SSLGuacamoleProperty or PrivateKeyGuacamoleProperty or something like that...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141361788
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -105,4 +146,59 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+ private final String decryptPassword(String encryptedPassword)
+ throws GuacamoleException {
+
+ // If we get nothing, we return nothing.
+ if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+ logger.warn("No or empty encrypted password, no password will be available.");
+ return null;
+ }
+
+ final PrivateKey clearpassKey = confService.getClearpassKey();
+ if (clearpassKey == null) {
+ logger.warn("No private key available to decrypt password.");
+ return null;
+ }
+
+ try {
+
+ final Cipher cipher = Cipher.getInstance(clearpassKey.getAlgorithm());
+
+ if (cipher == null)
+ throw new GuacamoleServerException("Failed to initialize cipher object with private key.");
+
+ // Initialize the Cipher in decrypt mode.
+ cipher.init(Cipher.DECRYPT_MODE, clearpassKey);
+
+ // Decode and decrypt, and return a new string.
+ final byte[] pass64 = DatatypeConverter.parseBase64Binary(encryptedPassword);
+ final byte[] cipherData = cipher.doFinal(pass64);
+ return new String(cipherData);
+
+ }
+ catch (Throwable t) {
--- End diff --
Okay, I'll go back to catching them each individually and go that route. Thanks.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141365440
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -105,4 +146,59 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+ private final String decryptPassword(String encryptedPassword)
+ throws GuacamoleException {
+
+ // If we get nothing, we return nothing.
+ if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+ logger.warn("No or empty encrypted password, no password will be available.");
+ return null;
+ }
+
+ final PrivateKey clearpassKey = confService.getClearpassKey();
+ if (clearpassKey == null) {
+ logger.warn("No private key available to decrypt password.");
+ return null;
+ }
+
+ try {
+
+ final Cipher cipher = Cipher.getInstance(clearpassKey.getAlgorithm());
+
+ if (cipher == null)
+ throw new GuacamoleServerException("Failed to initialize cipher object with private key.");
+
+ // Initialize the Cipher in decrypt mode.
+ cipher.init(Cipher.DECRYPT_MODE, clearpassKey);
+
+ // Decode and decrypt, and return a new string.
+ final byte[] pass64 = DatatypeConverter.parseBase64Binary(encryptedPassword);
+ final byte[] cipherData = cipher.doFinal(pass64);
+ return new String(cipherData);
+
+ }
+ catch (Throwable t) {
--- End diff --
Should be good - let me know if any of the error messages need tweaking.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r140011656
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java ---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements GuacamoleProperty<Cipher> {
+
+ @Override
+ public Cipher parseValue(String value) throws GuacamoleException {
+
+ try {
+
+ final Environment environment = new LocalEnvironment();
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(environment.getGuacamoleHome(), value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ final byte[] keyBytes = new byte[(int) keyFile.length()];
+ keyInput.read(keyBytes);
+ keyInput.close();
+
+ // Set up decryption infrastructure
+ KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+ KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+ final PrivateKey privateKey = keyFactory.generatePrivate(keySpec);
+ final Cipher cipher = Cipher.getInstance(privateKey.getAlgorithm());
+ cipher.init(Cipher.DECRYPT_MODE, privateKey);
+
+ return cipher;
+
+ }
+ catch (FileNotFoundException e) {
+ throw new GuacamoleException("Could not find the specified key file.", e);
+ }
+ catch (IOException e) {
+ throw new GuacamoleException("Could not read in the specified key file.", e);
+ }
+ catch (NoSuchAlgorithmException e) {
+ throw new GuacamoleException("Specified algorithm does not exist.", e);
+ }
+ catch (InvalidKeyException e) {
+ throw new GuacamoleException("Specified key is invalid.", e);
+ }
+ catch (InvalidKeySpecException e) {
+ throw new GuacamoleException("Invalid KeySpec initialization.", e);
+ }
+ catch (NoSuchPaddingException e) {
+ throw new GuacamoleException("No such padding exception.", e);
+ }
+
--- End diff --
> ... so instead of returning null, here, I'm throwing GuacamoleException.
Sounds good. I'm always in favor of strict error handling, though [`GuacamoleServerException`](http://guacamole.incubator.apache.org/doc/guacamole-common/org/apache/guacamole/GuacamoleServerException.html) may be a better choice.
> This means that, if those exceptions are thrown while CAS is using this property, it'll kill CAS authentication if this Cipher property fails to parse for one of these reasons. I consider that behavior less desirable than just not having the password available to the user, as authentication has technically still succeeded.
Is it expected that this will occasionally fail under normal (not misconfigured) conditions? If not, I'd think the administrator would rather be able to rely on all-or-nothing, predictable behavior, particularly if relying on this feature.
> I suppose I could capture the GuacamoleException inside the CAS module when pulling this property and just return null instead of allowing the GuacamoleException to pass through...
That would depend on which is worse: having a partially-functional system, where explicitly-enabled functionality is mysteriously non-functional, or having a disabled system, where authentication fails entirely until everything which has been explicitly enabled is actually functional.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141266837
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -83,7 +115,15 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
String ticket = request.getParameter(CASTicketField.PARAMETER_NAME);
if (ticket != null) {
AuthenticatedUser authenticatedUser = authenticatedUserProvider.get();
- authenticatedUser.init(ticketService.processUsername(ticket), credentials);
+ AttributePrincipal principal = ticketService.validateTicket(ticket);
+ String username = principal.getName();
+ Object credObj = principal.getAttributes().get("credential");
+ if (credObj != null) {
--- End diff --
The complexity of what's going on here is increasing significantly. Thoughts on encapsulating this credential retrieval logic within the ticket service, for maintainability/readability's sake?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r135426202
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java ---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements GuacamoleProperty<Cipher> {
+
+ @Override
+ public Cipher parseValue(String value) throws GuacamoleException {
+
+ try {
+
+ final Environment environment = new LocalEnvironment();
+
--- End diff --
This feels like reverting back to behavior advised against before, but I'm not sure the best way to get the Guacamole Home directory into this property to open up the file without doing this. Suggestions?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134759742
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -83,7 +106,16 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
String ticket = request.getParameter(CASTicketField.PARAMETER_NAME);
if (ticket != null) {
AuthenticatedUser authenticatedUser = authenticatedUserProvider.get();
- authenticatedUser.init(ticketService.processUsername(ticket), credentials);
+ AttributePrincipal principal = ticketService.validateTicket(ticket);
+ String username = principal.getName();
+ credentials.setUsername(username);
+ Object credObj = principal.getAttributes().get("credential");
+ if (credObj != null) {
+ String clearPass = decryptPassword(credObj.toString());
--- End diff --
Is there a defined object type for what `get("credential")` returns? Or is conversion to a string and back again the only way?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134765996
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ * @throws GuacamoleException
--- End diff --
Fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142019015
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java ---
@@ -57,21 +57,21 @@
private ConfigurationService confService;
/**
- * Validates and parses the given ID ticket, returning the Credentials object
- * derived from the parameters provided by the CAS server in the ticket. If the
+ * Validates and parses the given ID ticket, returning the username
+ * provided by the CAS server in the ticket. If the
* ticket is invalid an exception is thrown.
*
* @param ticket
* The ID ticket to validate and parse.
*
* @return
- * The Credentials object derived from parameters provided in the ticket.
+ * The username derived from the ticket.
*
* @throws GuacamoleException
* If the ID ticket is not valid or guacamole.properties could
* not be parsed.
*/
- public Credentials validateTicket(String ticket) throws GuacamoleException {
+ public String validateTicket(String ticket, Credentials credentials) throws GuacamoleException {
--- End diff --
The new `credentials` parameter needs to be documented, too.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r140645414
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/CipherGuacamoleProperty.java ---
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class CipherGuacamoleProperty implements GuacamoleProperty<Cipher> {
+
+ @Override
+ public Cipher parseValue(String value) throws GuacamoleException {
+
+ try {
+
+ final Environment environment = new LocalEnvironment();
+
--- End diff --
Absolute path is fine - I think there are a few other similar properties (basic user mapping, even) that do that, so it isn't without precedent. I guess it just seems like assuming it'll be in GUACAMOLE_HOME feels a little cleaner from a configuration perspective - admins don't have to keep guessing which properties assume absolutely paths and which are relative to GUACAMOLE_HOME - but as long as it's properly documented it should be okay.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141375864
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -83,7 +115,15 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
String ticket = request.getParameter(CASTicketField.PARAMETER_NAME);
if (ticket != null) {
AuthenticatedUser authenticatedUser = authenticatedUserProvider.get();
- authenticatedUser.init(ticketService.processUsername(ticket), credentials);
+ AttributePrincipal principal = ticketService.validateTicket(ticket);
+ String username = principal.getName();
+ Object credObj = principal.getAttributes().get("credential");
+ if (credObj != null) {
--- End diff --
Okay, I've moved a bunch of the decryption logic over to the TicketValidationService class and am returning a CredentialsObject back to the AuthenticationProviderService, instead. How does that look?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142019767
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java ---
@@ -36,30 +46,35 @@
public class TicketValidationService {
/**
+ * Logger for this class.
+ */
+ private static final Logger logger = LoggerFactory.getLogger(TicketValidationService.class);
+
+ /**
* Service for retrieving CAS configuration information.
*/
@Inject
private ConfigurationService confService;
/**
- * Validates and parses the given ID ticket, returning the username contained
- * therein, as defined by the username claim type given in
- * guacamole.properties. If the username claim type is missing or the ID
- * ticket is invalid, an exception is thrown instead.
+ * Validates and parses the given ID ticket, returning the username
+ * provided by the CAS server in the ticket. If the
+ * ticket is invalid an exception is thrown.
*
* @param ticket
* The ID ticket to validate and parse.
+ * @param credentials
--- End diff --
Mind adding the blank line above `@param credentials` to match the style of other comments?
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r143890795
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ int keyLength = (int) keyFile.length();
+ final byte[] keyBytes = new byte[keyLength];
+ int keyRead = keyInput.read(keyBytes);
+
+ // Error reading any bytes out of the key.
+ if (keyRead == -1)
+ throw new GuacamoleServerException("Failed to get any bytes while reading key.");
+
+ // Zero-sized key
+ else if(keyRead == 0)
+ throw new GuacamoleServerException("Failed to ready key because key is empty.");
+
+ // Fewer bytes read than contained in the key
+ else if (keyRead < keyLength)
--- End diff --
When reading in data from a file, while it's not guaranteed that each read will fill the buffer, returning fewer bytes than desired does not mean that future reads will fail. As long as end-of-file hasn't been reached (`read()` returns -1), and no error has been indicated (`IOException`), then `read()` should continue to be called until the buffer has been filled.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r144834201
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ int keyLength = (int) keyFile.length();
+ final byte[] keyBytes = new byte[keyLength];
--- End diff --
I figured the property was generic enough that it might be useful to some other purpose in the future, so I put it in the guacamole-ext area instead of CAS. I can move it if it makes sense at the moment - it can always be moved back up in the future if someone else finds it useful.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141267616
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -105,4 +146,59 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ *
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+ private final String decryptPassword(String encryptedPassword)
+ throws GuacamoleException {
+
+ // If we get nothing, we return nothing.
+ if (encryptedPassword == null || encryptedPassword.isEmpty()) {
+ logger.warn("No or empty encrypted password, no password will be available.");
+ return null;
+ }
+
+ final PrivateKey clearpassKey = confService.getClearpassKey();
+ if (clearpassKey == null) {
+ logger.warn("No private key available to decrypt password.");
+ return null;
+ }
+
+ try {
+
+ final Cipher cipher = Cipher.getInstance(clearpassKey.getAlgorithm());
+
+ if (cipher == null)
+ throw new GuacamoleServerException("Failed to initialize cipher object with private key.");
+
+ // Initialize the Cipher in decrypt mode.
+ cipher.init(Cipher.DECRYPT_MODE, clearpassKey);
+
+ // Decode and decrypt, and return a new string.
+ final byte[] pass64 = DatatypeConverter.parseBase64Binary(encryptedPassword);
+ final byte[] cipherData = cipher.doFinal(pass64);
+ return new String(cipherData);
+
+ }
+ catch (Throwable t) {
--- End diff --
Yeah, catching `Throwable` is generally bad. There are cases where it's necessary, but they're not common. Best practice is normally to explicitly catch the exceptions which apply, catching superclasses of those exceptions where it makes sense to do so.
It doesn't have to be pointless - if the context of each relevant exception is known, you can include a meaningful message in the `GuacamoleServerException` which you use to wrap the caught exception, rather than simply rethrowing things.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141266516
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java ---
@@ -68,4 +70,20 @@ public String getRedirectURI() throws GuacamoleException {
return environment.getRequiredProperty(CASGuacamoleProperties.CAS_REDIRECT_URI);
}
+ /**
+ * Returns the path to the file that contains the private key
--- End diff --
This function returns a `PrivateKey`, not a path.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r147473323
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.cas.conf;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.properties.GuacamoleProperty;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ FileInputStream keyStreamIn = new FileInputStream(keyFile);
+ ByteArrayOutputStream keyStreamOut = new ByteArrayOutputStream();
+ byte[] keyBuffer = new byte[1024];
+ try {
+ for (int readBytes; (readBytes = keyStreamIn.read(keyBuffer)) != -1;)
+ keyStreamOut.write(keyBuffer, 0, readBytes);
+ }
+ catch (IOException e) {
+ throw new GuacamoleServerException("IOException while trying to read bytes from file.", e);
--- End diff --
"IOException while trying to read bytes" will only have meaning to a developer. Any reason why this error is being caught here, rather than letting it get caught by the identical catch which comes later? The message for that other catch, "could not read in the specified key file", is much more clear.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134762077
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
--- End diff --
My rationale here was that it's actually okay, and should not halt the login process, if either the credential object is not available, or it can't be decrypted. Failure to retrieve or decrypt this property, IMHO, should not stop the login process from working or connections from being made - the token simply won't be available. Is there an exception that should be thrown that would not cause the login process to stop?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134757155
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
--- End diff --
All exceptions are actually caught and masked by that function, with any failures whatsoever resulting in return of `null`. Why not rethrow those failures as `GuacamoleServerException` or similar?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r147473937
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.cas.conf;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.properties.GuacamoleProperty;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ FileInputStream keyStreamIn = new FileInputStream(keyFile);
+ ByteArrayOutputStream keyStreamOut = new ByteArrayOutputStream();
+ byte[] keyBuffer = new byte[1024];
+ try {
+ for (int readBytes; (readBytes = keyStreamIn.read(keyBuffer)) != -1;)
+ keyStreamOut.write(keyBuffer, 0, readBytes);
+ }
+ catch (IOException e) {
+ throw new GuacamoleServerException("IOException while trying to read bytes from file.", e);
--- End diff --
Ummm...I wasn't paying attention to the fact that I already had it there?! Removed.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134767686
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ * @throws GuacamoleException
+ * If unable to get Guacamole configuration data
+ */
+ private final String decryptPassword(String encryptedPassword)
+ throws GuacamoleException {
+
+ // If we get nothing, we return nothing.
+ if (encryptedPassword == null || encryptedPassword.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(new LocalEnvironment().getGuacamoleHome(), confService.getClearpassKey().toString());
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ final byte[] keyBytes = new byte[(int) keyFile.length()];
+ keyInput.read(keyBytes);
+ keyInput.close();
+
+ // Set up decryption infrastructure
+ KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+ KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+ final PrivateKey privateKey = keyFactory.generatePrivate(keySpec);
+ final Cipher cipher = Cipher.getInstance(privateKey.getAlgorithm());
+ final byte[] pass64 = DatatypeConverter.parseBase64Binary(encryptedPassword);
--- End diff --
Added this exception to the list.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r144761726
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ int keyLength = (int) keyFile.length();
+ final byte[] keyBytes = new byte[keyLength];
--- End diff --
Oh.
Why is `PrivateKeyGuacamoleProperty` being added to the public extension API, rather than being internal to the CAS extension?
I'm not necessarily against adding a new property type at the extension API level, but PKCS8 + RSA is rather specific.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r143891620
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ int keyLength = (int) keyFile.length();
+ final byte[] keyBytes = new byte[keyLength];
--- End diff --
Beware that if you pre-allocate the `byte[]` like this, we'll need to bail out if the file size differs from what is expected. I don't think that's bad necessarily, so long as the case is handled.
The alternative would be to dynamically allocate the `byte[]`. An easy way would be to use something like a [`ByteArrayOutputStream`](https://docs.oracle.com/javase/7/docs/api/java/io/ByteArrayOutputStream.html), reading/writing chunks in a loop.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r134758567
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -105,4 +137,76 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Takes an encrypted string representing a password provided by
+ * the CAS ClearPass service and decrypts it using the private
+ * key configured for this extension. Returns null if it is
+ * unable to decrypt the password.
+ *
+ * @param encryptedPassword
+ * A string with the encrypted password provided by the
+ * CAS service.
+ *
+ * @return
+ * The decrypted password, or null if it is unable to
+ * decrypt the password.
+ * @throws GuacamoleException
--- End diff --
Missing blank line separating the `@return` block from the `@throws`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r142019823
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ final byte[] keyBytes = new byte[(int) keyFile.length()];
+ keyInput.read(keyBytes);
--- End diff --
`read()` isn't guaranteed to read in 100% of the bytes requested, nor is it guaranteed to succeed. The return value of `read()` will need to be handled if you wish to reliably read the key file into the byte array.
See: https://docs.oracle.com/javase/7/docs/api/java/io/InputStream.html#read()
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141926972
--- Diff: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java ---
@@ -82,8 +87,17 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
if (request != null) {
String ticket = request.getParameter(CASTicketField.PARAMETER_NAME);
if (ticket != null) {
+ Credentials ticketCredentials = ticketService.validateTicket(ticket);
+ if (ticketCredentials != null) {
+ String username = ticketCredentials.getUsername();
+ if (username != null)
+ credentials.setUsername(username);
+ String password = ticketCredentials.getPassword();
+ if (password != null)
+ credentials.setPassword(password);
+ }
AuthenticatedUser authenticatedUser = authenticatedUserProvider.get();
- authenticatedUser.init(ticketService.processUsername(ticket), credentials);
+ authenticatedUser.init(credentials.getUsername(), credentials);
--- End diff --
This is dangerous. Consider the following flow:
1. A malicious user sets the `username` query parameter to a known user (say, "guacadmin"), knowing that Guacamole will automatically populate the `Credentials` object with that value.
2. `ticketService.validateTicket()` does not set the username due to `principal.getName()` returning `null`, thus the username of `credentials` remains unchanged.
3. After CAS authentication completes, the user is given the identity of the user they specified, rather than the identity verified by CAS.
The identity of the user should come purely from CAS.
I would suggest:
* Passing the main `Credentials` object into `validateTicket()`, relying on `validateTicket()` to handle assignment of username/password values.
* Going back to simply returning the username, rather than a `Credentials` object which must be manually copied into the main `Credentials` object, trusting *only* the value which comes from CAS.
---
[GitHub] incubator-guacamole-client pull request #183: GUACAMOLE-362: Add support for...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/183#discussion_r141266397
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/properties/PrivateKeyGuacamoleProperty.java ---
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import java.io.BufferedInputStream;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.InputStream;
+import java.io.IOException;
+import java.lang.IllegalArgumentException;
+import java.security.InvalidKeyException;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PrivateKey;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+import java.security.spec.PKCS8EncodedKeySpec;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.environment.Environment;
+import org.apache.guacamole.environment.LocalEnvironment;
+
+/**
+ * A GuacamoleProperty whose value is derived from a private key file.
+ */
+public abstract class PrivateKeyGuacamoleProperty implements GuacamoleProperty<PrivateKey> {
+
+ @Override
+ public PrivateKey parseValue(String value) throws GuacamoleServerException {
+
+ if (value == null || value.isEmpty())
+ return null;
+
+ try {
+
+ // Open and read the file specified in the configuration.
+ File keyFile = new File(value);
+ InputStream keyInput = new BufferedInputStream(new FileInputStream(keyFile));
+ final byte[] keyBytes = new byte[(int) keyFile.length()];
+ keyInput.read(keyBytes);
+ keyInput.close();
+
+ // Set up decryption infrastructure
+ KeyFactory keyFactory = KeyFactory.getInstance("RSA");
+ KeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes);
+ return keyFactory.generatePrivate(keySpec);
+
+ }
+ catch (FileNotFoundException e) {
+ throw new GuacamoleServerException("Could not find the specified key file.", e);
+ }
+ catch (IOException e) {
+ throw new GuacamoleServerException("Could not read in the specified key file.", e);
+ }
+ catch (NoSuchAlgorithmException e) {
+ throw new GuacamoleServerException("Specified algorithm does not exist.", e);
+ }
+ catch (InvalidKeySpecException e) {
+ throw new GuacamoleServerException("Invalid KeySpec initialization.", e);
--- End diff --
Is there a more meaningful error message for the failure which results in this exception?
---