Implement read and parse JSON structured secrets from AWS secretsmanager

[froehr]

Hey,

I used your Jenkins plugin to get secrets from AWS into Jenkins. It was really helpful but I felt it would be useful if it was possible to also use secrets that contain key value pairs and are therefor stored in a json structure.

• Implemented functionality to read JSON structured secrets.
  • To use a secret the secret name and the json key in the object need to be provided like secretName>jsonKey
• > is used as a delimiter as . or : did not work. (see below)
• The functionality is explained in the README.md.
• Tests are implemented.

I had rather used . or : as a delimiter instead of > but . is an available character in a secret name and : is filtered out internally by Jenkins for some reason. If you have another preference please let me know.

I would really like to hear your feedback on it. If you like it please merge it so that others can profit from it as well.

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests - that demonstrates feature works or fixes the issue

[chriskilding]

Hi, glad to hear the plugin is working for you and thanks for the PR :)

I believe AWS has a particular syntax for referencing a key-value pair inside a JSON secret. This is used e.g. in CloudFormation templates. Off the top of my head I think it's something like:

{{ resolve:secretsmanager:<secret>:SecretString:<KEY> }}

There's variations on that to allow for referencing secret versions too.

I think we should follow their convention if possible so that it's more intuitive for users. Let's try to figure out why the colon delimiter didn't work 🤔

[froehr]

Hey,
I think using their convention would be the right way to go. I've looked into how the resolving process works under the hood.

Internally a class called InterpolatorStringLookup extends AbstractStringLookup is used in the process to look up the secret. In the end the reveal method in AwsSecretsManagerSecretSource is used but a function called lookup in InterpolatorStringLookup changes the input that the reveal method gets. It has a hardcoded piece of code:

private static final char PREFIX_SEPARATOR = ':';

It is used to remove a potential prefix. This is a problem as in our case we are interested in this prefix. I don't think it can be disabled but maybe you can take a look as well.

The method that alters the input looks as follows:

public String lookup(String var) {
        if (var == null) {
            return null;
        }
        final int prefixPos = var.indexOf(PREFIX_SEPARATOR);
        if (prefixPos >= 0) {
            final String prefix = toKey(var.substring(0, prefixPos));
            final String name = var.substring(prefixPos + 1);
            final StringLookup lookup = stringLookupMap.get(prefix);
            String value = null;
            if (lookup != null) {
                value = lookup.lookup(name);
            }

            if (value != null) {
                return value;
            }
            var = var.substring(prefixPos + 1);
        }
        if (defaultStringLookup != null) {
            return defaultStringLookup.lookup(var);
        }
        return null;
}

In this case var could be : secretName:someKey
As mentioned before PREFIX_SEPARATOR is :

return defaultStringLookup.lookup(var); only forwards someKey to AwsSecretsManagerSecretSource

[chriskilding]

I'm wondering if the prefix in CasC is used to indicate the type of resolver or transformer that should be used... did you see anything around the lookup method that would suggest that's the case?

Docs here: https://github.com/jenkinsci/configuration-as-code-plugin/blob/master/docs/features/secrets.adoc#additional-variable-substitution

Example in a unit test: https://github.com/jenkinsci/configuration-as-code-plugin/blob/2d26bbfb301d6d402270f00d7f99913aefa26493/plugin/src/test/java/io/jenkins/plugins/casc/SecretSourceResolverTest.java#L229

[froehr]

Yes, the prefix is used to determine what type of secret is resolved. It is a hardcoded map which contains base64, fileBase64, readFileBase64, file, readFile and decodeBase64. As far as I can tell it cannot be changed dynamically. If no prefix is set it falls back to String resolver. In our case it finds the prefix but as it is not part of that list it is ignored:

This comes from the Java Doc:

Resolves the specified variable. This implementation will try to extract a variable prefix from the given 
variable name (the first colon (':') is used as prefix separator). It then passes the name of the variable 
with the prefix stripped to the lookup object registered for this prefix. If no prefix can be found or if the 
associated lookup object cannot resolve this variable, the default lookup object will be used.

In addition we use io.jenkins.plugins.casc.SecretSource as an extension point like described here: https://www.jenkins.io/doc/developer/extensions/configuration-as-code/#secretsource I think it only takes in a string an nothing more.

Right now I don't see a way to make this work without changing the underlying CasC Plugin.

[froehr]

In addition, do you understand why Jenkins is using this plugin in the end? Is it just going through all potential sources and hoping that one will resolve it? I don't really understand how this is determined.

[chriskilding]

Yes, the SecretSource resolver essentially does what you say - it iterates through all installed SecretSource implementations, until one of them successfully resolves the secret ID.

I guess this works because in most cases there will be very few implementations installed - probably just the local disk/machine secret resolver, and one remote resolver (like this plugin). It would, however, get 'fun' if you were working in a multi-cloud setup and had e.g. the AWS SecretSource plus an Azure SecretSource installed.

[froehr]

Ok, that makes sense but definitely also creates challenges.

What do you think this means for this PR? Do you think you would like to merge it?

[chriskilding]

I think the next thing to do would be to raise an issue with the CasC plugin saying what we'd like to achieve, the obstacle that the CasC plugin currently presents, and ask if the CasC authors can see a way to change the CasC plugin to let implementations use : for their own purposes.

[froehr]

Something came to my mind when reading the documentation of the CasC plugin again to be able to open a proper issue. I think we actually don't really need a change as only the first prefix is filtered out. Please have a look at my latest change. I think it works like this as well.

[chriskilding]

I'm curious about 2 things:

• What happens in the regular version of the plugin if you use the secret ARN, rather than the secret name, in the casc.yaml? (This isn't something I've had any occasion to do, but I believe that the AWS SDK's GetSecretValue call builder allows an ARN in the name field.)
• And what happens if you use the secret ARN plus the extension for referencing a key within the secret (e.g. arn:aws:secretsmanager:us-west-2:123456789012:secret:MySecret123:SecretString:FOO) in the casc.yaml?

[froehr]

I guess that in the current plugin version 0.0.2 the following would happen:

A user would pass the arn via CasC.yaml to the plugin like this: ${arn:aws:secretsmanager:us-west-2:123456789012:secret:MySecret123}

As the CasC plugin drops the first "prefix" before the first : only aws:secretsmanager:us-west-2:123456789012:secret:MySecret123 would end up in the plugin. This would probably fail as it is not a complete arn.

In the new approach you'd pass ${aws-secretsmanager:arn:aws:secretsmanager:us-west-2:123456789012:secret:MySecret123} to the CasC.yaml. In return the plugin would receive arn:aws:secretsmanager:us-west-2:123456789012:secret:MySecret123 as an input. Unfortunately this would also fail as we use the : as a delimiter to split key and value and we'd search for a secret called arn on aws.

If we'd want to circumvent this we'd need a more complex Regex to match the arn pattern and react on it.

[chriskilding]

What I'm thinking is that - rather than use aws-secretsmanager as a custom prefix - we'd say that if you want to use JSON secrets, you would specify the secret ARN (plus the secret version and key to extract in the AWS format), rather than the secret name, in the casc.yaml.

We'd then parse the name to check if it starts with arn:aws:secretsmanager, which has the advantage of being a standard prefix rather than our own one.

If casc-plugin drops the arn: prefix, that's okay because we can still look for aws:secretsmanager:... which should still be present in the string handed to us. We can then reinstate the arn: prefix and do the secret lookup.

[froehr]

I've just implemented this idea because I think it's actually very clever. Thanks.
One thing that came to my mind right after finishing it was that using the arn could actually make it a bit harder to use in the end. When Jenkins is deployed on different stages like test and prod and the arn changes due to the different account id the casc.yaml has to be changed per stage. What do you think about this concern?

In addition it could potentially cause the assumption that it is possible to read from secrets from different AWS accounts.

[chriskilding]

Now you mention it... if you've got access to a cross-account setup, could you check if providing an ARN from another account - and also providing the AWS auth config to do the cross-account login - works?

[froehr]

Sorry, I don't have access to multiple AWS accounts.

[chriskilding]

In that case no worries.

Regarding the fact that the account ID would be embedded in the ARN, and therefore need to change between environments - I think this is okay because in a multi-account setup, casc.yaml is usually generated with configuration management (e.g. Terraform), so the account ID can be fed in as a template variable.

[chriskilding]

I think the thing to do at this point is to see if the CasC plugin can make things nicer for us - perhaps, after doing its preprocessing thing, it could provide us an option to access either (a) the original secret name or (b) the original secret prefix (which, being arn:, it should not have done anything with). I'll raise an issue.

(This would avoid us having to make the small assumption that something which starts aws:secretsmanager... does indeed need arn: prepended to it. It's not a huge assumption, but it would be good to eliminate assumptions from the design if possible.)

[chriskilding]

Raised in jenkinsci/configuration-as-code-plugin#1949

[chriskilding]

Hmm... are you sure that the arn: prefix is filtered out by the time it gets to our implementation?

The author of casc-plugin has said that the arn: prefix should be preserved, based on the way it works: jenkinsci/configuration-as-code-plugin#1949 (comment)

[froehr]

Yes, I'm sure the prefix is filtered out. This might be a bug if the author thinks it shouldn't. Please check out my branch and run the unit test shouldRevealSecretValueWhenPlainTextSecretIsReferencedViaArn. If you put a breakpoint into the reveal() method you will see that the prefix is missing.

[chriskilding]

I've let the author of casc-plugin know about the test case showing that the prefix is removed - hopefully we'll see a response from him soon.

[astrahan87]

@froehr @chriskilding Any updates on the status of this change?
This is something I'd like to be able to leverage, but it doesn't look like there's been any response regarding the issue with the casc-plugin.

[froehr]

I think all necessary inputs are provided and we don't have it in our hands right now. From my point of view we could also just ignore the problem for now and make it a little bit less according to the standard as it will not work until fixed in the casc plugin.

[chriskilding]

I've provided the CasC maintainers with a failing test on casc-plugin itself that demonstrates the problem, as they asked.

jenkinsci/configuration-as-code-plugin#2007

Hopefully we can now work towards a fix upstream.

[jetersen]

@chriskilding and @froehr you would need to accept the next bom version that would include the fix for jcasc, I will make sure we get a release out for the bom :)

[jetersen]

Yes, the SecretSource resolver essentially does what you say - it iterates through all installed SecretSource implementations, until one of them successfully resolves the secret ID.

If you have any idea how to improve the secret source extension resolver but again I think it would be hard to figure out who deserves to resolve things.

Potentially we could enable an extension to modify the hashmap used within the prefix lookup. But that might not be the best solution.

[chriskilding]

@jetersen will that BOM release be available for 2.289 (which is what this plugin currently uses) or will we need to bump?

[jetersen]

just waiting on rerun of bom and we should have a release of it.

[jetersen]

Release inc https://github.com/jenkinsci/bom/runs/7091074572?check_suite_focus=true
Yes bom 2.289 should include the update 👍

[jetersen]

@chriskilding you should be able to head here and force Dependabot to check dependencies now.
https://github.com/jenkinsci/aws-secrets-manager-secret-source-plugin/network/updates

https://github.com/jenkinsci/bom/releases/tag/1461.vb_3c6de28f2b_a_
https://repo.jenkins-ci.org/releases/io/jenkins/tools/bom/bom-2.289.x/1461.vb_3c6de28f2b_a_/

[jetersen]

You should depend on the Jenkins jackson API plugin.
The version is defined in the plugin bill of material.

[chriskilding]

The updated BOM (with the new configuration-as-code-plugin containing the fix) is in main, rebasing this branch now...

[chriskilding]

Nearly there - tests refactored, new BOM changes integrated. Just need to tidy up the implementation and we should get to the finish line.

[chriskilding]

One thing I would like to know is any qualifiers that might exist on an ARN that we haven't thought about yet. For example, can an ARN contain the secret version as a suffix/qualifier? If so that might cause trouble for the JSON key detection logic, which relies on the JSON key being the only thing in the ARN qualifier.

[chriskilding]

Newsflash...

I've been working to add JSON secret support to configuration-as-code-plugin. This has just been released in the latest version 1466.v2d4119502006.

This means it's no longer necessary to add dedicated JSON parsing logic to the SecretSource plugin; it's all handled by the json: secret postprocessor in CasC instead.

You should now be able to parse a JSON secret using the latest casc plugin, and the standard version of this plugin, like this...

If you have a secret called my-secret in Secrets Manager as follows:

{ "username": "foo", "password": "bar" }

Then reference the secret, together with the key to extract, like this:

jenkins:
  securityRealm:
    local:
      allowsSignup: false
      users:
      - id: "${json:username:${my-secret}}"
        password: "${json:password:${my-secret}}"

Give this a go and let me know if it works for you

[chriskilding]

(Incidentally, because this logic has been hoisted into the CasC plugin, this means you can now parse a JSON secret from any secrets backend - whether that's AWS, Azure, Kubernetes, or something else.)

[jetersen]

So basically this PR is obsolete with the new JCasC feature? 👏

[chriskilding]

I'm hoping so!

[chriskilding]

@froehr have you been able to evaluate the new JSON secret support in configuration-as-code-plugin?

[chriskilding]

I have written a replacement PR #101 which includes the instructions for using the CasC json helper.

I think the instructions should be sufficient so if I don't hear anything in the next week or two, I'll close this PR, and merge #101, on 15th August.