You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2022/07/01 23:34:57 UTC
[GitHub] [guacamole-client] jmuehlner opened a new pull request, #741: GUACAMOLE-1372: Support signing/encryption keys for SAML auth extension.
jmuehlner opened a new pull request, #741:
URL: https://github.com/apache/guacamole-client/pull/741
https://issues.apache.org/jira/browse/GUACAMOLE-1372
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #741: GUACAMOLE-1372: Support signing/encryption keys for SAML auth extension.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #741:
URL: https://github.com/apache/guacamole-client/pull/741#discussion_r914148948
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java:
##########
@@ -329,6 +366,69 @@ public int getAuthenticationTimeout() throws GuacamoleException {
return environment.getProperty(SAML_AUTH_TIMEOUT, 5);
}
+ /**
+ * Returns the file containing the x509 certificate to use when signing
+ * requests to the SAML IdP. If the property is not set, null will be
+ * returned.
+ *
+ * @return
+ * The file containing the x509 certificate to use when signing
+ * requests to the SAML IdP, or null if not defined.
+ *
+ * @throws GuacamoleException
+ * If the authentication timeout cannot be parsed.
+ */
+ public File getCertificateFile() throws GuacamoleException {
+ return environment.getProperty(SAML_X509_CERT_PATH);
+ }
+
+ /**
+ * Returns the file containing the private key to use when signing
+ * requests to the SAML IdP. If the property is not set, null will be
+ * returned.
+ *
+ * @return
+ * The file containing the private key to use when signing
+ * requests to the SAML IdP, or null if not defined.
+ *
+ * @throws GuacamoleException
+ * If the authentication timeout cannot be parsed.
+ */
+ public File getPrivateKeyFile() throws GuacamoleException {
+ return environment.getProperty(SAML_PRIVATE_KEY_PATH);
+ }
+
+ /**
+ * Returns the contents of a small file, such as a private key or certificate into
+ * a String. If the file does not exist, or cannot be read for any reason, a warning
+ * will be logged and null will be returned.
+ *
+ * @param file
+ * The file to read into a string.
+ *
+ * @param name
+ * A human-readable name for the file, to be used when formatting log messages.
+ *
+ * @return
+ * The contents of the file having the given path, or null if the file does not
+ * exist or cannot be read.
+ */
+ private String readFileContentsIntoString(File file, String name) {
+
+ // Attempt to read the file directly into a String
+ try {
+ return new String(Files.readAllBytes(file.toPath()), StandardCharsets.UTF_8);
+ }
+
+ // If the file cannot be read, log a warning and treat it as if it does not exist
+ catch (IOException e) {
+ logger.warn("{} \"{}\" could not be read.", name, file);
Review Comment:
Am I not already doing that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a diff in pull request #741: GUACAMOLE-1372: Support signing/encryption keys for SAML auth extension.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on code in PR #741:
URL: https://github.com/apache/guacamole-client/pull/741#discussion_r914132272
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java:
##########
@@ -329,6 +366,69 @@ public int getAuthenticationTimeout() throws GuacamoleException {
return environment.getProperty(SAML_AUTH_TIMEOUT, 5);
}
+ /**
+ * Returns the file containing the x509 certificate to use when signing
+ * requests to the SAML IdP. If the property is not set, null will be
+ * returned.
+ *
+ * @return
+ * The file containing the x509 certificate to use when signing
+ * requests to the SAML IdP, or null if not defined.
+ *
+ * @throws GuacamoleException
+ * If the authentication timeout cannot be parsed.
Review Comment:
X.509 certificate*
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java:
##########
@@ -329,6 +366,69 @@ public int getAuthenticationTimeout() throws GuacamoleException {
return environment.getProperty(SAML_AUTH_TIMEOUT, 5);
}
+ /**
+ * Returns the file containing the x509 certificate to use when signing
+ * requests to the SAML IdP. If the property is not set, null will be
+ * returned.
+ *
+ * @return
+ * The file containing the x509 certificate to use when signing
+ * requests to the SAML IdP, or null if not defined.
+ *
+ * @throws GuacamoleException
+ * If the authentication timeout cannot be parsed.
+ */
+ public File getCertificateFile() throws GuacamoleException {
+ return environment.getProperty(SAML_X509_CERT_PATH);
+ }
+
+ /**
+ * Returns the file containing the private key to use when signing
+ * requests to the SAML IdP. If the property is not set, null will be
+ * returned.
+ *
+ * @return
+ * The file containing the private key to use when signing
+ * requests to the SAML IdP, or null if not defined.
+ *
+ * @throws GuacamoleException
+ * If the authentication timeout cannot be parsed.
Review Comment:
private key file*
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java:
##########
@@ -329,6 +366,69 @@ public int getAuthenticationTimeout() throws GuacamoleException {
return environment.getProperty(SAML_AUTH_TIMEOUT, 5);
}
+ /**
+ * Returns the file containing the x509 certificate to use when signing
+ * requests to the SAML IdP. If the property is not set, null will be
+ * returned.
+ *
+ * @return
+ * The file containing the x509 certificate to use when signing
+ * requests to the SAML IdP, or null if not defined.
+ *
+ * @throws GuacamoleException
+ * If the authentication timeout cannot be parsed.
+ */
+ public File getCertificateFile() throws GuacamoleException {
+ return environment.getProperty(SAML_X509_CERT_PATH);
+ }
+
+ /**
+ * Returns the file containing the private key to use when signing
+ * requests to the SAML IdP. If the property is not set, null will be
+ * returned.
+ *
+ * @return
+ * The file containing the private key to use when signing
+ * requests to the SAML IdP, or null if not defined.
+ *
+ * @throws GuacamoleException
+ * If the authentication timeout cannot be parsed.
+ */
+ public File getPrivateKeyFile() throws GuacamoleException {
+ return environment.getProperty(SAML_PRIVATE_KEY_PATH);
+ }
+
+ /**
+ * Returns the contents of a small file, such as a private key or certificate into
+ * a String. If the file does not exist, or cannot be read for any reason, a warning
+ * will be logged and null will be returned.
+ *
+ * @param file
+ * The file to read into a string.
+ *
+ * @param name
+ * A human-readable name for the file, to be used when formatting log messages.
+ *
+ * @return
+ * The contents of the file having the given path, or null if the file does not
+ * exist or cannot be read.
+ */
+ private String readFileContentsIntoString(File file, String name) {
+
+ // Attempt to read the file directly into a String
+ try {
+ return new String(Files.readAllBytes(file.toPath()), StandardCharsets.UTF_8);
+ }
+
+ // If the file cannot be read, log a warning and treat it as if it does not exist
+ catch (IOException e) {
+ logger.warn("{} \"{}\" could not be read.", name, file);
Review Comment:
We should log the exception message here, as well, so that the admin can see the nature of the failure. For example:
https://github.com/apache/guacamole-client/blob/754e9649f1fa0ba225ee42b56ded64bc283d17df/guacamole/src/main/java/org/apache/guacamole/tunnel/InterceptedStreamMap.java#L71-L74
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java:
##########
@@ -161,6 +174,30 @@ public class ConfigurationService {
};
+ /**
+ * The file containing the x509 cert to use when signing or encrypting
+ * requests to the SAML IdP.
+ */
+ private static final FileGuacamoleProperty SAML_X509_CERT_PATH =
+ new FileGuacamoleProperty() {
+
+ @Override
+ public String getName() { return "saml-x509-cert-path"; }
+
+ };
+
+ /**
+ * The file containing the private to use when signing or encrypting
Review Comment:
private key*
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java:
##########
@@ -329,6 +366,69 @@ public int getAuthenticationTimeout() throws GuacamoleException {
return environment.getProperty(SAML_AUTH_TIMEOUT, 5);
}
+ /**
+ * Returns the file containing the x509 certificate to use when signing
+ * requests to the SAML IdP. If the property is not set, null will be
+ * returned.
+ *
+ * @return
+ * The file containing the x509 certificate to use when signing
+ * requests to the SAML IdP, or null if not defined.
+ *
+ * @throws GuacamoleException
+ * If the authentication timeout cannot be parsed.
+ */
+ public File getCertificateFile() throws GuacamoleException {
+ return environment.getProperty(SAML_X509_CERT_PATH);
+ }
+
+ /**
+ * Returns the file containing the private key to use when signing
+ * requests to the SAML IdP. If the property is not set, null will be
+ * returned.
+ *
+ * @return
+ * The file containing the private key to use when signing
+ * requests to the SAML IdP, or null if not defined.
+ *
+ * @throws GuacamoleException
+ * If the authentication timeout cannot be parsed.
+ */
+ public File getPrivateKeyFile() throws GuacamoleException {
+ return environment.getProperty(SAML_PRIVATE_KEY_PATH);
+ }
+
+ /**
+ * Returns the contents of a small file, such as a private key or certificate into
+ * a String. If the file does not exist, or cannot be read for any reason, a warning
+ * will be logged and null will be returned.
+ *
+ * @param file
+ * The file to read into a string.
+ *
+ * @param name
+ * A human-readable name for the file, to be used when formatting log messages.
+ *
+ * @return
+ * The contents of the file having the given path, or null if the file does not
+ * exist or cannot be read.
+ */
+ private String readFileContentsIntoString(File file, String name) {
+
+ // Attempt to read the file directly into a String
+ try {
+ return new String(Files.readAllBytes(file.toPath()), StandardCharsets.UTF_8);
+ }
+
+ // If the file cannot be read, log a warning and treat it as if it does not exist
+ catch (IOException e) {
+ logger.warn("{} \"{}\" could not be read.", name, file);
+ logger.debug("{} \"{}\" could not be read.", name, file, e);
+ return null;
Review Comment:
Should this instead be fatal, ultimately causing the call to `getSamlSettings()` to fail if required files can't be read?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a diff in pull request #741: GUACAMOLE-1372: Support signing/encryption keys for SAML auth extension.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on code in PR #741:
URL: https://github.com/apache/guacamole-client/pull/741#discussion_r914182526
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java:
##########
@@ -329,6 +366,69 @@ public int getAuthenticationTimeout() throws GuacamoleException {
return environment.getProperty(SAML_AUTH_TIMEOUT, 5);
}
+ /**
+ * Returns the file containing the x509 certificate to use when signing
+ * requests to the SAML IdP. If the property is not set, null will be
+ * returned.
+ *
+ * @return
+ * The file containing the x509 certificate to use when signing
+ * requests to the SAML IdP, or null if not defined.
+ *
+ * @throws GuacamoleException
+ * If the authentication timeout cannot be parsed.
+ */
+ public File getCertificateFile() throws GuacamoleException {
+ return environment.getProperty(SAML_X509_CERT_PATH);
+ }
+
+ /**
+ * Returns the file containing the private key to use when signing
+ * requests to the SAML IdP. If the property is not set, null will be
+ * returned.
+ *
+ * @return
+ * The file containing the private key to use when signing
+ * requests to the SAML IdP, or null if not defined.
+ *
+ * @throws GuacamoleException
+ * If the authentication timeout cannot be parsed.
+ */
+ public File getPrivateKeyFile() throws GuacamoleException {
+ return environment.getProperty(SAML_PRIVATE_KEY_PATH);
+ }
+
+ /**
+ * Returns the contents of a small file, such as a private key or certificate into
+ * a String. If the file does not exist, or cannot be read for any reason, a warning
+ * will be logged and null will be returned.
+ *
+ * @param file
+ * The file to read into a string.
+ *
+ * @param name
+ * A human-readable name for the file, to be used when formatting log messages.
+ *
+ * @return
+ * The contents of the file having the given path, or null if the file does not
+ * exist or cannot be read.
+ */
+ private String readFileContentsIntoString(File file, String name) {
+
+ // Attempt to read the file directly into a String
+ try {
+ return new String(Files.readAllBytes(file.toPath()), StandardCharsets.UTF_8);
+ }
+
+ // If the file cannot be read, log a warning and treat it as if it does not exist
+ catch (IOException e) {
+ logger.warn("{} \"{}\" could not be read.", name, file);
+ logger.debug("{} \"{}\" could not be read.", name, file, e);
+ return null;
Review Comment:
They're not required, yes, but when the admin _does_ specify them, they're declaring that they want the SAML messages to be signed/encrypted using that key. If the admin explicitly requests that messages be signed/encrypted with a particular key, but that key can't be read, I think we should fail outright rather than fall back to sending unsigned/unencrypted messages.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a diff in pull request #741: GUACAMOLE-1372: Support signing/encryption keys for SAML auth extension.
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on code in PR #741:
URL: https://github.com/apache/guacamole-client/pull/741#discussion_r914182526
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java:
##########
@@ -329,6 +366,69 @@ public int getAuthenticationTimeout() throws GuacamoleException {
return environment.getProperty(SAML_AUTH_TIMEOUT, 5);
}
+ /**
+ * Returns the file containing the x509 certificate to use when signing
+ * requests to the SAML IdP. If the property is not set, null will be
+ * returned.
+ *
+ * @return
+ * The file containing the x509 certificate to use when signing
+ * requests to the SAML IdP, or null if not defined.
+ *
+ * @throws GuacamoleException
+ * If the authentication timeout cannot be parsed.
+ */
+ public File getCertificateFile() throws GuacamoleException {
+ return environment.getProperty(SAML_X509_CERT_PATH);
+ }
+
+ /**
+ * Returns the file containing the private key to use when signing
+ * requests to the SAML IdP. If the property is not set, null will be
+ * returned.
+ *
+ * @return
+ * The file containing the private key to use when signing
+ * requests to the SAML IdP, or null if not defined.
+ *
+ * @throws GuacamoleException
+ * If the authentication timeout cannot be parsed.
+ */
+ public File getPrivateKeyFile() throws GuacamoleException {
+ return environment.getProperty(SAML_PRIVATE_KEY_PATH);
+ }
+
+ /**
+ * Returns the contents of a small file, such as a private key or certificate into
+ * a String. If the file does not exist, or cannot be read for any reason, a warning
+ * will be logged and null will be returned.
+ *
+ * @param file
+ * The file to read into a string.
+ *
+ * @param name
+ * A human-readable name for the file, to be used when formatting log messages.
+ *
+ * @return
+ * The contents of the file having the given path, or null if the file does not
+ * exist or cannot be read.
+ */
+ private String readFileContentsIntoString(File file, String name) {
+
+ // Attempt to read the file directly into a String
+ try {
+ return new String(Files.readAllBytes(file.toPath()), StandardCharsets.UTF_8);
+ }
+
+ // If the file cannot be read, log a warning and treat it as if it does not exist
+ catch (IOException e) {
+ logger.warn("{} \"{}\" could not be read.", name, file);
+ logger.debug("{} \"{}\" could not be read.", name, file, e);
+ return null;
Review Comment:
They're not required, yes, but when the admin _does_ specify them, they're declaring that they want the SAML messages to be signed/encrypted using that key. If the admin explicitly requests that messages be signed/encrypted with a particular key, but that key can't be read, I think we should fail outright rather than continue as if no key was provided.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #741: GUACAMOLE-1372: Support signing/encryption keys for SAML auth extension.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #741:
URL: https://github.com/apache/guacamole-client/pull/741#discussion_r914157873
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java:
##########
@@ -329,6 +366,69 @@ public int getAuthenticationTimeout() throws GuacamoleException {
return environment.getProperty(SAML_AUTH_TIMEOUT, 5);
}
+ /**
+ * Returns the file containing the x509 certificate to use when signing
+ * requests to the SAML IdP. If the property is not set, null will be
+ * returned.
+ *
+ * @return
+ * The file containing the x509 certificate to use when signing
+ * requests to the SAML IdP, or null if not defined.
+ *
+ * @throws GuacamoleException
+ * If the authentication timeout cannot be parsed.
+ */
+ public File getCertificateFile() throws GuacamoleException {
+ return environment.getProperty(SAML_X509_CERT_PATH);
+ }
+
+ /**
+ * Returns the file containing the private key to use when signing
+ * requests to the SAML IdP. If the property is not set, null will be
+ * returned.
+ *
+ * @return
+ * The file containing the private key to use when signing
+ * requests to the SAML IdP, or null if not defined.
+ *
+ * @throws GuacamoleException
+ * If the authentication timeout cannot be parsed.
+ */
+ public File getPrivateKeyFile() throws GuacamoleException {
+ return environment.getProperty(SAML_PRIVATE_KEY_PATH);
+ }
+
+ /**
+ * Returns the contents of a small file, such as a private key or certificate into
+ * a String. If the file does not exist, or cannot be read for any reason, a warning
+ * will be logged and null will be returned.
+ *
+ * @param file
+ * The file to read into a string.
+ *
+ * @param name
+ * A human-readable name for the file, to be used when formatting log messages.
+ *
+ * @return
+ * The contents of the file having the given path, or null if the file does not
+ * exist or cannot be read.
+ */
+ private String readFileContentsIntoString(File file, String name) {
+
+ // Attempt to read the file directly into a String
+ try {
+ return new String(Files.readAllBytes(file.toPath()), StandardCharsets.UTF_8);
+ }
+
+ // If the file cannot be read, log a warning and treat it as if it does not exist
+ catch (IOException e) {
+ logger.warn("{} \"{}\" could not be read.", name, file);
+ logger.debug("{} \"{}\" could not be read.", name, file, e);
+ return null;
Review Comment:
Should it? They're not required properties.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper merged pull request #741: GUACAMOLE-1372: Support signing/encryption keys for SAML auth extension.
Posted by GitBox <gi...@apache.org>.
mike-jumper merged PR #741:
URL: https://github.com/apache/guacamole-client/pull/741
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #741: GUACAMOLE-1372: Support signing/encryption keys for SAML auth extension.
Posted by GitBox <gi...@apache.org>.
jmuehlner commented on code in PR #741:
URL: https://github.com/apache/guacamole-client/pull/741#discussion_r914183553
##########
extensions/guacamole-auth-sso/modules/guacamole-auth-sso-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java:
##########
@@ -329,6 +366,69 @@ public int getAuthenticationTimeout() throws GuacamoleException {
return environment.getProperty(SAML_AUTH_TIMEOUT, 5);
}
+ /**
+ * Returns the file containing the x509 certificate to use when signing
+ * requests to the SAML IdP. If the property is not set, null will be
+ * returned.
+ *
+ * @return
+ * The file containing the x509 certificate to use when signing
+ * requests to the SAML IdP, or null if not defined.
+ *
+ * @throws GuacamoleException
+ * If the authentication timeout cannot be parsed.
+ */
+ public File getCertificateFile() throws GuacamoleException {
+ return environment.getProperty(SAML_X509_CERT_PATH);
+ }
+
+ /**
+ * Returns the file containing the private key to use when signing
+ * requests to the SAML IdP. If the property is not set, null will be
+ * returned.
+ *
+ * @return
+ * The file containing the private key to use when signing
+ * requests to the SAML IdP, or null if not defined.
+ *
+ * @throws GuacamoleException
+ * If the authentication timeout cannot be parsed.
+ */
+ public File getPrivateKeyFile() throws GuacamoleException {
+ return environment.getProperty(SAML_PRIVATE_KEY_PATH);
+ }
+
+ /**
+ * Returns the contents of a small file, such as a private key or certificate into
+ * a String. If the file does not exist, or cannot be read for any reason, a warning
+ * will be logged and null will be returned.
+ *
+ * @param file
+ * The file to read into a string.
+ *
+ * @param name
+ * A human-readable name for the file, to be used when formatting log messages.
+ *
+ * @return
+ * The contents of the file having the given path, or null if the file does not
+ * exist or cannot be read.
+ */
+ private String readFileContentsIntoString(File file, String name) {
+
+ // Attempt to read the file directly into a String
+ try {
+ return new String(Files.readAllBytes(file.toPath()), StandardCharsets.UTF_8);
+ }
+
+ // If the file cannot be read, log a warning and treat it as if it does not exist
+ catch (IOException e) {
+ logger.warn("{} \"{}\" could not be read.", name, file);
+ logger.debug("{} \"{}\" could not be read.", name, file, e);
+ return null;
Review Comment:
Required now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org