Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GUACAMOLE-641: Add support for populating arbitrary parameter tokens from key vaults. #336

Closed
wants to merge 15 commits into from

Conversation

mike-jumper
Copy link
Contributor

@mike-jumper mike-jumper commented Oct 28, 2018

These changes add a new family of extensions, similar to "guacamole-auth-jdbc", which provide support for retrieval of secrets from key vaults: "guacamole-vault". Initial support for Azure Key Vault is present through the "guacamole-vault-azure" module, with the necessary structure in place to allow other implementations to be provided in the future.

The general support works as follows:

  1. A YAML file is included within GUACAMOLE_HOME which defines a token/secret mapping. Besides defining tokens, the names of each secret may also contain tokens which allow the secret name to vary by connection ID, hostname, username, etc. There is a specific set of tokens available for use within secret names.
  2. When a user attempts to connect to a connection or connection group, the key vault implementation is queried to retrieve each defined secret. Tokens are then defined for only those secrets whose names are fully defined within the current context and which have values stored within the vault.
  3. The extension defining the connection is given these tokens. If the extension supports substitution of parameter tokens, as all current extensions do, those tokens will be substituted within the configuration of the connection. Guacamole extensions are not required to do this, however.
  4. To avoid excessive retrieval requests, the result of retrieving a particular secret is cached (by default for 10 seconds).

This thus allows secret values like passwords and private keys to be stored off-site within the vault and retrieved dynamically based on context.

Copy link
Contributor

@necouchman necouchman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial review, with some minor questions/comments and a couple of minor things to fix, but, unsurprisingly, overall it looks good. Worth noting that I do not have a way to test it...

mike-jumper added a commit to mike-jumper/guacamole-client that referenced this pull request Dec 14, 2018
…iom.

From apache#336 (comment):

>
> SLF4J formerly recommended that instance variables be used
> (non-static), but no longer takes either stance:
> https://www.slf4j.org/faq.html#declared_static
> 
> If we have to pick something to be the standard going forward, I'd
> say let's stick with the accepted idiom of `private static final`
> loggers, with the exception being where it's actually necessary to
> not be `static` (dependency injection).
>
Copy link
Contributor

@necouchman necouchman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay in getting back to reviewing this. 6 months goes by quick. A few more comments and items to fix up...

@mike-jumper mike-jumper changed the base branch from master to staging/1.4.0 August 30, 2021 16:48
mike-jumper added a commit to mike-jumper/guacamole-client that referenced this pull request Dec 18, 2021
…iom.

From apache#336 (comment):

>
> SLF4J formerly recommended that instance variables be used
> (non-static), but no longer takes either stance:
> https://www.slf4j.org/faq.html#declared_static
> 
> If we have to pick something to be the standard going forward, I'd
> say let's stick with the accepted idiom of `private static final`
> loggers, with the exception being where it's actually necessary to
> not be `static` (dependency injection).
>
@mike-jumper mike-jumper marked this pull request as draft December 18, 2021 06:40
@mike-jumper
Copy link
Contributor Author

OK - I believe I've resolved all feedback thus far, as well as brought things up to date. I also:

  • Updated to the latest versions of the libraries.
  • Switched from JSON to YAML for the token/secret mapping file.
  • Renamed from guacamole-auth-vault-* to guacamole-vault-* (since it's not providing any kind of auth).
  • Renamed the only other non-auth extension (guacamole-auth-quickconnect) following the same logic.
  • Leveraged getPrivileged() for parameter retrieval, so that it's more likely that we'll be able to vary things based on connection hostname and connection username.

BUT, I've also converted this to draft because of difficulties in obtaining the license text from the "azure-annotations" dependency. To be clear, the pom.xml for that dependency clearly states that the library is MIT-licensed, as does the source for that library, however the full text of the license file can't currently be obtained because the corresponding git repository is private (presumably by mistake). See:

/**
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License. See License.txt in the project root for
* license information.
*/
NOTE: The above has been extracted from the source of the "azure-annotations"
library, as may be downloaded from Maven Central:
https://search.maven.org/remotecontent?filepath=com/microsoft/azure/azure-annotations/1.10.0/azure-annotations-1.10.0-sources.jar
Unfortunately, the "License.txt" file noted is not included with the source
.jar, and the GitHub repository referenced by the pom.xml of
"azure-annotations" is not publicly visible:
https://github.com/Microsoft/java-api-annotations
I (Mike Jumper) have reached out to Microsoft to correct this and to request a
copy of the "License.txt" file if access to this repository cannot be fixed in
the near future. Until then, the above should serve as reasonable confirmation
that this library is indeed (1) licensed under the MIT license and (2)
copyright Microsoft Corporation.
For reference, the terms of the open source license widely known as the "MIT
license" can be found here:
https://opensource.org/licenses/MIT

As noted in the placeholder above, I've already emailed Microsoft about this, and they are tracking things down, but are unsure whether this will be resolved soon due to the holidays. I've asked whether they can send us the License.txt file manually for the time being so that we can move forward. We shall see...

Copy link
Contributor

@necouchman necouchman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, a few more comments while you wait for the feedback from Microsoft....or you don't get any feedback and we just have to pick a route and go for it.

mike-jumper added a commit to mike-jumper/guacamole-client that referenced this pull request Dec 26, 2021
…iom.

From apache#336 (comment):

>
> SLF4J formerly recommended that instance variables be used
> (non-static), but no longer takes either stance:
> https://www.slf4j.org/faq.html#declared_static
> 
> If we have to pick something to be the standard going forward, I'd
> say let's stick with the accepted idiom of `private static final`
> loggers, with the exception being where it's actually necessary to
> not be `static` (dependency injection).
>
@mike-jumper mike-jumper force-pushed the key-vault branch 2 times, most recently from ba32262 to 3559a76 Compare December 26, 2021 10:40
…cationiProviderModule" should be "AzureKeyVaultAuthenticationProviderModule".
…iom.

From apache#336 (comment):

>
> SLF4J formerly recommended that instance variables be used
> (non-static), but no longer takes either stance:
> https://www.slf4j.org/faq.html#declared_static
> 
> If we have to pick something to be the standard going forward, I'd
> say let's stick with the accepted idiom of `private static final`
> loggers, with the exception being where it's actually necessary to
> not be `static` (dependency injection).
>
…avoid confusion with "GUAC_USERNAME".

The "GUAC_USERNAME" token provided by the webapp is based off the
username provided by the user when they authenticated. The username
token provided by the vault extensions uses the username stored with
the user's corresponding object, which may not be the same.
…ens only if corresponding parameters are non-empty.
@asfgit asfgit deleted the branch apache:staging/1.4.0 January 2, 2022 05:15
@asfgit asfgit closed this Jan 2, 2022
mike-jumper added a commit to mike-jumper/guacamole-client that referenced this pull request Jan 21, 2022
…iom.

From apache#336 (comment):

>
> SLF4J formerly recommended that instance variables be used
> (non-static), but no longer takes either stance:
> https://www.slf4j.org/faq.html#declared_static
> 
> If we have to pick something to be the standard going forward, I'd
> say let's stick with the accepted idiom of `private static final`
> loggers, with the exception being where it's actually necessary to
> not be `static` (dependency injection).
>
mike-jumper added a commit to mike-jumper/guacamole-client that referenced this pull request Jan 21, 2022
…iom.

From apache#336 (comment):

>
> SLF4J formerly recommended that instance variables be used
> (non-static), but no longer takes either stance:
> https://www.slf4j.org/faq.html#declared_static
>
> If we have to pick something to be the standard going forward, I'd
> say let's stick with the accepted idiom of `private static final`
> loggers, with the exception being where it's actually necessary to
> not be `static` (dependency injection).
>
@mike-jumper mike-jumper deleted the key-vault branch January 22, 2022 01:22
mike-jumper added a commit to mike-jumper/guacamole-client that referenced this pull request Jan 23, 2022
…iom.

From apache#336 (comment):

>
> SLF4J formerly recommended that instance variables be used
> (non-static), but no longer takes either stance:
> https://www.slf4j.org/faq.html#declared_static
>
> If we have to pick something to be the standard going forward, I'd
> say let's stick with the accepted idiom of `private static final`
> loggers, with the exception being where it's actually necessary to
> not be `static` (dependency injection).
>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants