secret names with slashes are not viewable from credentials manager (404 error) about aws-secrets-manager-credentials-provider-plugin

[chriskilding]

Started a PR to fix this: #218

At the moment that PR is very much non-functional while we try to figure out how the CredentialsWrapper is used in the bigger picture of the CredentialsStore.

[chriskilding]

I've written a test case for this here: #206

Unfortunately I am unable to reproduce the error, as the test has worked just fine. Would you be able to take the test case from that PR and tweak it, to see if you can reproduce it?

[pjdarton]

I have the same issue as reported here on my Jenkins instance too.

The credentials work as in builds are able to use them, and they show up when visiting the credentials view https://<your-jenkins>/credentials, e.g.

... but you can't click on one of those credentials and see the details in Jenkins. e.g. clicking on "Secret for github" says:

However, if I hand-edit the URL to replace the / in the credential name with %2F then it works:

I've taken a look at the unit test added in #206 and it seems that it merely checks that a credential is usable (it shows up in the lists that builds can choose from), not that the credential can be inspected using the Jenkins credentials admin Web UI.

I think that what's needed is a unit-test that has a secret called foo/bar with description "FooBar" and verifies that the https://<your-jenkins>/credentials URL returns a page containing a link which has text FooBar pointing to a URL ending /foo%2Fbar/ instead of /foo/bar/

[chriskilding]

The Secrets Manager provider isn't the only plugin in the chain, so I'm just wondering if maybe the URL encoding handling is going wrong upstream...

Could you try creating a local credential in the default (on disk) credentials provider with the name that you used before (/i2eng/cicd...), then try browsing to it in the same way, and let me know if that fails due to the same error?

[pjdarton]

I think this says it all...

If you use the built-in credentials provider, it tries to stop you you creating credentials with a / in the ID.
The AWS credentials provider doesn't stop you and, in many ways, folks are encouraged to use this kind of path with AWS secrets, thus causing this issue...

...however, if I ignore the red warning and create it anyway (with username "foo", password "bar" and description "Foo/bar") then it gets created successfully:

...and the link on that page to edit the credential works OK - it has all the / characters replaced with %2F in the URL already.
The fact that it worked despite the red warning suggests that the red warning was a false-alarm (so maybe there was a time when it wasn't a false-alarm and there's been code added to support / in the path since?)

HOWEVER, if I then browse back to just /credentials/:

...then the link there does NOT work - that page doesn't have the / replaced with %2F and clicking it causes a 404.

i.e. the basic credential provider partly works with IDs containing a / - when listing credentials for the built-in provider's global credential then the links work, but when listing "all credentials in Jenkins" then the links don't.

...but if I inspect the AWS provider's global credentials then the links don't work there (or on the "all credentials in Jenkins" page).

TL;DR: The links are 50% borked with the built-in Jenkins credential provider but 100% borked with the AWS provider. i.e. it's a bit of a mess...

[chriskilding]

Yes, we'll need to go digging in the default credential provider code to see if it has special handling for / characters. Not only to see what they did, but also to reuse it if we can - it's important to be consistent with that handling code to make future maintenance easier.

[chriskilding]

This is the bit of credentials-plugin that causes the validation warning you saw:

public final FormValidation doCheckId(@ContextInPath ModelObject context, @QueryParameter String value) {
            if (value.isEmpty()) {
                return FormValidation.ok();
            }
            if (!value.matches("[a-zA-Z0-9_.-]+")) {
                return FormValidation.error("Unacceptable characters");
            }
// ....

https://github.com/jenkinsci/credentials-plugin/blob/589d6391ed041908a49023033d7982f71dbc1042/src/main/java/com/cloudbees/plugins/credentials/impl/BaseStandardCredentials.java#L201

It's not targeting the '/' character in particular, and there's nothing in the associated git commit to suggest it was. Rather, it's a general limitation to ASCII alphanumeric plus a couple of delimiters they decided to allow (underscores, periods, or hyphens).

[pjdarton]

Yup, and there's even an old JIRA report complaining that it's demanding "a too narrow set of characters", https://issues.jenkins.io/browse/JENKINS-45254
...but that JIRA notes "Sadly the source code does not indicate any reasons why this regex was added in the first place." 😞

My guess is that there is a bug in the credential plugin where it fails to correctly encode the URL in one of the two places where credentials are listed ... but it gets it right in the other place, and we need to figure out how that works so that the AWS plugin can copy that trick.
(and if that then reveals how/why it's not working with the top-level view then that's help get that one fixed in the credentials plugin too ... or maybe the answer lies 100% within the credentials plugin?)

[chriskilding]

If I had to guess, I imagine it was added as a basic 'sanitise your inputs' (OWASP bread and butter secure coding) measure. In that school of thought, the fewer characters you allow the better, and I guess few people were using '/' as a namespacer back then.

The hunt continues for the url encoding bit in credentials-plugin... (alternatively it could be upstream in credentials-api)

[pjdarton]

The problem is that it doesn't "sanitise [your] inputs" because it doesn't actually prevent creation of credentials with illegal IDs.
Sure, it displays a red error complaining about it, but the "submit" button works and creates the credential with the nasty ID.
i.e. it adds inconvenience and worries the user, but doesn't provide any security by policing the IDs.
Plus, of course, the basic credential provider can't stop other credential providers (like this AWS one) from creating IDs that the basic provider wouldn't like.

(credentials-api? There's another layer too? Well, I guess it's worth a look, given that I got nowhere fast with the credentials plugin itself...)

[chriskilding]

Yes, credentials-api is the plugin that contains the credentials interfaces, which credentials consumers interact with.

credentials-plugin, aws-secrets-manager-credentials-provider-plugin and so forth contain the concrete providers which implement those interfaces.

[chriskilding]

It's possible that URL encoding is being handled by a proxy class or method in credentials-api

[chriskilding]

The nested class CredentialsStoreAction$CredentialsWrapper in credentials-plugin (that's the concrete implementation) looks promising...

It overrides the describable's method public String getUrlName(), and performs some kind of encoding on the credential ID.

If indeed this is it, it explains how it could simultaneously urlencode the credential ID for the Web UI, and make the unmodified credential ID available to direct consumers (because if everything received a urlencoded ID, they would probably break).

Edit: Link is https://github.com/jenkinsci/credentials-plugin/blob/b03fbc98f341564f8e663c01184a9f9a8e3891a5/src/main/java/com/cloudbees/plugins/credentials/CredentialsStoreAction.java#L1150

/**
     * A wrapper object to bind and expose {@link Credentials} instances into the web UI.
     */
    @ExportedBean
    public static class CredentialsWrapper extends AbstractDescribableImpl<CredentialsWrapper>
            implements IconSpec, ModelObjectWithContextMenu, AccessControlled {

        /**
         * Our {@link DomainWrapper}.
         */
        private final DomainWrapper domain;

        /**
         * The {@link Credentials} that we are wrapping.
         */
        private final Credentials credentials;

        /**
         * The {@link IdCredentials#getId()} of the {@link Credentials}.
         */
        private final String id;
        private Fingerprint fingerprint;

        /**
         * Constructor.
         *
         * @param domain      the wrapped domain.
         * @param credentials the credentials.
         * @param id          the id.
         */
        public CredentialsWrapper(DomainWrapper domain, Credentials credentials, String id) {
            this.domain = domain;
            this.credentials = credentials;
            this.id = id;
        }

        /**
         * Return the id for the XML API.
         *
         * @return the id.
         * @since 2.1.0
         */
        @Exported
        public String getId() {
            return id;
        }

        /**
         * Return the URL name.
         *
         * @return the URL name.
         */
        public String getUrlName() {
            return Util.rawEncode(id);
        }

...

[Peter-Darton-i2]

That seems a plausible explanation 👍

...but only for one of the two cases - it doesn't explain why / characters can mess things up for one of the two credential-listings - I suspect one of them is calling getId() and the other is calling getUrlName()...

[chriskilding]

I have asked the author of Azure Keyvault Plugin whether that credentials provider supports IDs with / characters, and if so how they are urlencoded.

[chriskilding]

jenkinsci/azure-keyvault-plugin#115

[chriskilding]

It turns out that Azure Keyvault Plugin conveniently sidesteps this problem, because in Azure Keyvault secrets are not allowed to have / in their IDs anyway.

This is because the URL structure of Keyvault works like S3; there's one global namespace:

https://{vault-name}.vault.azure.net/{secret-type}/{secret-name}/{secret-version}

And vault-name is typically your product name, with an environment suffix e.g. myproduct-staging, byproduct-production.

[chriskilding]

I've also opened https://issues.jenkins.io/browse/JENKINS-68817 to ask if it would be appropriate to move the CredentialsWrapper logic up into the Credentials API, so that urlencoding is done in a uniform way regardless of which provider is used.

[chriskilding]

Hi @Peter-Darton-i2

I circled back to this today to see if anything had changed in more recent Jenkins versions, and Jenkins Credentials (API) versions.

I tried to create a local secret in the default credentials provider (so not the AWS one) called foo/bar. Like before, it rejected this because '/' is an invalid character.

I went ahead and created it with a different name, then shut Jenkins down, edited the credential's name in credentials.xml to be foo/bar, and started Jenkins again.

You can see that foo/bar is visible in the list:

HOWEVER, when you click on foo/bar to browse to it, you now get a 404 Not Found (the same error that you saw with credentials from Secrets Manager):

And changing the URL to escape the forward slash (http://localhost:8080/.../credential/foo%2Fbar/) makes it work again - just like you saw with the Secrets Manager plugin before.

This was seen with:

• Jenkins 2.361.1
• Credentials Plugin 1189.vf61b_a_5e2f62e

[chriskilding]

I think that if the default credentials provider is doing this, then this is now the standard expected behaviour across all providers. It's not something that can be handled within the Secrets Manager plugin, it would need to be dealt with upstream.

Fortunately this is only a minor issue for the Secrets Manager plugin, because the detailed credential view is mostly used to reach its editing form. In the Secrets Manager plugin credentials are read-only, so they are never edited within Jenkins anyway. It therefore doesn't block any critical paths of operation. (But this is a bigger issue for the default credentials provider which does rely on the editing form.)

I will therefore close this issue, but before I do, I think it's worth mentioning something about this in the README as a known limitation.

[chriskilding]

I have added this clarification to the README in #259

Although it's not exactly the fix you guys were hoping for (because that can only be implemented upstream), I hope that it will at least prevent confusion for future users of this plugin who encounter this behaviour. If you have any input on the documentation change PR, it would be appreciated.

[chriskilding]

The documentation clarification is now merged - closing this PR