search

jenkinsci / aws-secrets-manager-credentials-provider-plugin

AWS Secrets Manager Credentials Provider for Jenkins
https://plugins.jenkins.io/aws-secrets-manager-credentials-provider/
MIT License
65 stars 43 forks source link

Plugin throws exception for Secret File with SecretString #131

Open chriskilding opened 3 years ago

chriskilding commented 3 years ago

Reposted from https://issues.jenkins.io/browse/JENKINS-66588

I ran into JENKINS-62566 when trying to configure a SecretFile with the kubeconfig for a kubernetes cluster using the kubernetes plugin. This fails in an incredibly unobvious way, which took a lot of digging to troubleshoot.

Since the text of the exception is just the word null, the 'test config' button for the kubernetes cloud configuration page just displays the word 'null' in red text. This led me to believe the problem was in the kubernetes plugin's cloud component, but I couldn't figure out how it was throwing due to the exception not being logged at default log levels either. I originally assumed this was an issue with the kubeconfig file, or something wrong with cluster connectivity, until I'd ruled all that out successfully and created a new logger at ALL which showed the actual stacktrace in question, pointing to the AWS Secrets Manager credential provider plugin.

Finally, for ease of reproduction, I created a freestyle job in which I just used the file as a credential and saw the same stacktrace:

FATAL: null
java.lang.NullPointerException
    at io.jenkins.plugins.credentials.secretsmanager.factory.file.AwsFileCredentials.getContent(AwsFileCredentials.java:39)
    at org.jenkinsci.plugins.credentialsbinding.impl.FileBinding.write(FileBinding.java:54)
    at org.jenkinsci.plugins.credentialsbinding.impl.FileBinding.write(FileBinding.java:42)
    at org.jenkinsci.plugins.credentialsbinding.impl.AbstractOnDiskBinding.bindSingle(AbstractOnDiskBinding.java:39)
    at org.jenkinsci.plugins.credentialsbinding.Binding.bind(Binding.java:150)
    at org.jenkinsci.plugins.credentialsbinding.impl.SecretBuildWrapper.setUp(SecretBuildWrapper.java:87)
    at hudson.model.Build$BuildExecution.doRun(Build.java:158)
    at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:516)
    at hudson.model.Run.execute(Run.java:1889)
    at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
    at hudson.model.ResourceController.execute(ResourceController.java:100)
    at hudson.model.Executor.run(Executor.java:433)
Finished: FAILURE

While the documentation shows a FileCredential created by using awscli with the -secret-binary flag, it is not made obvious in the documentation that a SM secret created by using the AWS web console or the -secret-string flag to awscli is unsupported, and if I hadn't stumbled across JENKINS-62566 I would have no idea what was going wrong without a deep dive into the plugin source.

JENKINS-62566 was closed because it wasn't a common problem, but given how hard to troubleshoot it can be when credentials are used in system configuration, I'd love to see either a fallback where it gets the SecretString value if SecretBytes is null, or at the least, a more instructive stacktrace.

I'd think the SecretBytesSupplier.get() method would want to return SecretBytes.fromString(str) on line 71 here instead of returning null

I can try to toss together a PR for this with some test coverage if it's a wanted change; I'd think it would be expected that the contents of the secret will be available to people regardless of whether it is a binary or string.

menvol3 commented 3 years ago

Facing the same issue(

karthikkumarkp commented 3 years ago

facing same issue

chriskilding commented 2 years ago

I've been thinking about this for a bit...

The first thing to note is that if you need to pass a secret string to a build, by far the better way to do this is with the simple Secret Text.

The second thing is that the return type of Secret File is a binary field (SecretBytes). If the plugin accepted secret-string for Secret Files as well, it would have to make some internal decisions about how to translate the string to SecretBytes (e.g. hardcode a character set) which might be inappropriate in some use cases. To ensure the plugin behaves transparently, it is far better for the user to control the translation of string to binary on the client side, and have the plugin accept only secret-binary input for Secret Files.

Therefore I would favour:

Rather than adding secret-string support to the Secret File type.

r4v5 commented 2 years ago

A documentation fix where we show the user how to convert their secret string to a secret-binary format for upload (although I think this is no more complicated than echoing it to a file...)

Correct; if I remember correctly, the big thing that got me after figuring out it needed to be a SecretBinary was realizing the file also needed to be specified as fileb:// in the aws command (and learning that the aws command had a fileb protocol in the first place and it wasn't just a typo).

To ensure the plugin behaves transparently, it is far better for the user to control the translation of string to binary on the client side, and have the plugin accept only secret-binary input for Secret Files.

Without wading into the text conversion minefield/quicksand, there's several plugins that expect to receive a secret file credential specifically and to be able to use it as text within Jenkins (I have vague memories of the maven plugin being one of them when giving a custom .m2/settings.xml? and the aforementioned kubernetes plugin, which only supports Secret Text creds for tokens, not whole kubeconfigs).

In addition, all console-based credentials get added as SecretStrings, which means that people require AWS access keys and the CLI to upload if they need a Secret File credential.

How would you feel about the plugin making an assumption that a SecretString is deserializable as UTF8 and throwing if it's not? We'd want an INFO- or WARN-level log about the situation when the plugin has to do an automatic conversion so that those of us who miss that subtlety know how to make our credentials match the plugin best practices.

That way the plugin would handle what are likely the most common cases for the plugin to encounter a SecretString while still providing sufficient breadcrumbs for people to figure out their edge cases.

chriskilding commented 2 years ago

I believe that it might be as simple as passing your text file secret as the --secret-binary argument rather than the --secret-string.

Could you try running

aws secretsmanager create-secret --name '<secret name>' --secret-binary 'file://<your file>.txt' --tags 'Key=jenkins:credentials:type,Value=file' --description '<whatever>'

And then seeing what comes out when the secret file credential is utilised in Jenkins?

Side note: If you try to inspect this manually by doing

aws secretsmanager get-secret-value --secret-id <secret name> --query 'SecretBinary' --output text

in the CLI, you'll notice the output is base64 encoded. I believe this is only happening because the shell is a text-only environment, so AWI CLI must necessarily encode the binary output to show it. But the AWS SDK that we use in the Jenkins plugin returns us the byte stream without modification, so I'm hoping it will work.)