mike-jumper commented on a change in pull request #355: GUACAMOLE-271: Duo in
docker image
URL: https://github.com/apache/guacamole-client/pull/355#discussion_r247746775
##########
File path: guacamole-docker/bin/start.sh
##########
@@ -591,6 +591,18 @@ END
exit 1;
fi
+# Use Duo if specified
+if [ -n "$DUO_API_HOSTNAME" ] && \
+ [ -n "$DUO_INTEGRATION_KEY" ] && \
+ [ -n "$DUO_SECRET_KEY" ] && \
+ [ -n "$DUO_APPLICATION_KEY" ] ; then
+ set_optional_property "duo-api-hostname" "$DUO_API_HOSTNAME"
+ set_optional_property "duo-integration-key" "$DUO_INTEGRATION_KEY"
+ set_optional_property "duo-secret-key" "$DUO_SECRET_KEY"
Review comment:
These properties are actually all required, not optional:
http://guacamole.apache.org/doc/gug/duo-auth.html#guac-duo-config
If any are absent, the startup script should abort and warn the user, as
with the other extensions supported by the image. For example:
https://github.com/apache/guacamole-client/blob/9dcee8bdac2d40ae98ebabb53bdba2e9237ca93c/guacamole-docker/bin/start.sh#L128-L146
This should also be set up in a similar manner to the others. We currently
have extension-specific setup, parsing, etc. is organized within separate
functions, keeping the logic which follows later simple and unobstructed:
https://github.com/apache/guacamole-client/blob/9dcee8bdac2d40ae98ebabb53bdba2e9237ca93c/guacamole-docker/bin/start.sh#L551-L555
You're correct to not update the `INSTALLED_AUTH` environment variable, as
things will not work unless some other auth mechanism is set up, but things
will be easier to read and more maintainable if the existing pattern is
followed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
