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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
