Add credential management to custom files #90
Conversation
|
@imod any chance you could look this over especially since JENKINS-43204 is assigned to you? |
There was a problem hiding this comment.
I have not touched the code for quite a while so I'm not fully up to date anymore. But from the first look at it, it seems good. Just some one minor point...
I will try to get back to it once you have made the change, but to do a release, I need to reactivate my credentials first...
|
|
||
| public CustomConfigProvider() { | ||
| load(); | ||
| } | ||
|
|
||
| @Override | ||
| public ContentType getContentType() { | ||
| return null; | ||
| return ContentType.DefinedType.SHELL; |
There was a problem hiding this comment.
so per default new configs are shell-scripts?
There was a problem hiding this comment.
Well, it seems to make the most sense to make them "shell" type scripts. I'm leveraging the Token Macro plugin for replacing the variables and they're all shell type variables.
| import java.util.logging.Level; | ||
| import java.util.logging.Logger; | ||
|
|
||
| public class CredentialsHelper { |
There was a problem hiding this comment.
please rename to clearly separate from the org.jenkinsci.plugins.configfiles.maven.security.CredentialsHelper (specially as they also have similar methods). Maybe even rename both:
org.jenkinsci.plugins.configfiles.maven.security.MavenConfigCredentialsHelperorg.jenkinsci.plugins.configfiles.custom.security.CustomConfigCredentialsHelper
There was a problem hiding this comment.
I can definitely do that. I'll get a new version up later today.
There was a problem hiding this comment.
Ok, I've updated to org.jenkinsci.plugins.configfiles.custom.security.CustomConfigCredentialsHelper. I'll likely work on a separate commit for the maven class change.
|
@imod can you take a look again? My team is hoping to see this merged / released sometime soonish so that we can roll out new support options for our customers using it an JCasC! |
|
Bumping again, cause I don't want to maintain a fork. 🙏 |
|
@Aricg @tykeal sorry, but I just can't find the time anymore - this plugin is looking for a new maintainer https://groups.google.com/g/jenkinsci-dev/c/aCmm2MyydNA/m/_WogLPs7AgAJ |
|
@imod if you're ok with it, I'll put my name forward for maintainership of this plugin. I'll send a response to the dev list on the mail thread with the info that's noted in https://www.jenkins.io/doc/developer/plugin-governance/adopt-a-plugin/ |
|
I'm fairly sure this PR introduces a security vulnerability. I strongly recommend it not be merged. |
|
@daniel-beck I'll admit that this is my first time doing any sort of Java development in a very long time, but I'm failing to see where the security vulnerability is. In what way is it being introduced? I'm more than happy to work through any issues but I'm needing them pointed out. If it's because of the old version of plain-credentials plugin being added, my intent was to find the newest version of the plugin that worked with the existing code base so as to not have to do upgrades all over the place of dependencies first. Using the latest version of that plugin basically pushed me down a deep rabbit hole of updates all over the place in the plugin which was not the point of this PR |
|
@tykeal thanks for taking the lead! |
|
Hold on… moving the conversation to our private tracker. |
| public static class DescriptorImpl extends Descriptor<CustomizedCredentialMapping> { | ||
|
|
||
| public ListBoxModel doFillCredentialsIdItems(@AncestorInPath ItemGroup context, @QueryParameter String tokenKey) { | ||
| AccessControlled _context = (context instanceof AccessControlled ? (AccessControlled) context : Jenkins.get()); |
There was a problem hiding this comment.
This isn't following the standard pattern: https://github.com/jenkinsci/credentials-plugin/blob/master/docs/consumer.adoc#providing-a-ui-form-element-to-let-a-user-select-credentials
There was a problem hiding this comment.
@jvz ah... ok. I lifted some of that code from elsewhere in the plugin.
The maven and properties credential mappings both do it that way as well. I was sort of feeling my way around trying to get this to work given my examples in the plugin only dealt with the StandardUsernameCredentials and I also need to add in the StringCredentials from plain-credentials plugin.
There was a problem hiding this comment.
Mentioning here as I said this in private: the code here isn't necessarily wrong, but it's overly restrictive in its permission checks. The checks for Item.CONFIGURE should be replaced by checks to CredentialsProvider.USE_ITEM when validating whether or not credentials should be allowed to be used. This will allow Authorize Project to work properly here (otherwise, users with the ability to use credentials from a build as well as the permission to build the job would not be able to access the credentials when the job is run; the UseItem permission addresses this authorization issue). Essentially, a user who can build the job shouldn't also need permission to configure it just for the credentials to work (without Authorize Project installed, you wouldn't notice this problem since builds are executed as the system user normally, and that has permission to access almost any credentials).
I will note, though, that the doFill... and doCheck... methods for credentials can safely use Item.CONFIGURE as the permission check as that makes sense, but the code that resolves credentials ids into IdCredentials instances should certainly be checking the CredentialsProvider.USE_ITEM permission instead.
tykeal
force-pushed
the
tokenized
branch
2 times, most recently
from
aa11b93
to
6791cc4
Compare
|
@daniel-beck @imod @jvz I believe I've fixed all of your requests. |
|
Great, just need @daniel-beck and @imod to weigh in then :D (or at least @daniel-beck since he's the one with a change request on the change! |
|
Bumping. @daniel-beck and @imod but primarily @daniel-beck since his review is currently blocking any merge from what I can see. |
|
bumping again. @daniel-beck @imod what do I need to do to get this merged and a new release out? |
|
Bumping on this thread. I feel like a broken record here. Really trying to get this change merged since it's been nearly 5 months since I raised the initial PR and nearly 3 months since the last actual touch of code. I and my team would really like to see this get merged and a release with it happen. We don't really like installing self-hosted plugins into the Jenkins systems we manage, it makes it a lot harder for our customers to replicate our open infra if they need to for some reason! |
...main/java/org/jenkinsci/plugins/configfiles/custom/security/CustomizedCredentialMapping.java
Outdated
Show resolved
Hide resolved
...main/java/org/jenkinsci/plugins/configfiles/custom/security/CustomizedCredentialMapping.java
Outdated
Show resolved
Hide resolved
|
Actual build failure:
|
|
Looks like my ad-hoc suggestion was wrong, sorry about that. The correct behavior is to pass the same object that the permission is checked on. Once this is resolved, I have no further concerns about this PR. |
|
@daniel-beck ah, I wonder why it didn't blow up when I did my local build to make sure it still built. :-/ Ok, I'll revert back what I had. |
|
@daniel-beck ok, I've reverted the change I had made that you had requested. |
|
This original version is certainly wrong. My suggestion fails because of an unexpectedly restrictive annotation in Sadly we cannot pass |
|
https://gist.github.com/daniel-beck/4571f2849349c9bb16ad033b15c63016 should do it. It's not pretty, but should work as expected. Pinging @jvz and @jeffret-b who are maintainers of Credentials Plugin and may have a better idea how this API is supposed to be used without code duplication. |
|
That looks like it should work fine. |
With the advent of Jenkins Configuration As Code (JCasC) it has become more needed to be able to provide a way to allow job owners to provide new managed configuration files that also replace tokenized credentials. While it is possible to attain this goal by utilizing the Credentials Binding Plugin along with the 'Replace All' option when defining where the file is put on disk during the job this has caused several problems with well established methods of managing the jobs. In particular, jobs being managed with Jenkins Job Builder which tend to be heavily Freestyle in nature and also utilize a lot of templates and macros suffer greatly when using this method of management. Instead, allowing credentials to be defined and automatically replaced when the file is rendered down to disk solves the problem of securely storing the credentials in the credential store and also allowing less privileged persons the ability to more easily work with the managed files. This change in particular modifies the 'custom' file type to support defining credentials along with the tokens that are to be used and having them properly rendered when being put down on disk. It will continue to work fine with the previous method using the Credentials Binding Plugin as well as it will only replace the tokens that it has a full definition for and then would be passed through the full environment token replacement as well as it did before this change. Making this change allows for safe storage of configuration files in JCasC format to be stored in a repository and in some fashion manipulated into place for Jenkins to utilize (exercise for the admin) which is a partial answer to JENKINS-17766. It also completely obviates the requirement to encrypt the files as per JENKINS-18635 as the sensitive data can now be securely separated from the non-sensitive data. Resolves: JENKINS-43204 Signed-off-by: Andrew Grimberg <agrimberg@linuxfoundation.org>
|
@daniel-beck thanks for that. Yes, it works just fine and that code is definitely a lot better than what I had! |
|
@daniel-beck @imod I'm crying here... what do I need to do to get this over the finish line?? |
|
Daniel isn't a maintainer of this plugin. Maintainers listed are: @olamy @jglick @rsandell @cyrille-leclerc @imod @alecharp @varyvol @MRamonLeon |
|
If @daniel-beck, who has been working actively, approve this I'm happy to merge. Not sure if I may consider his thumbs-up in a comment as PR approved from my side. Probably it may take a week or a bit more because we're actively working on some important stuff on this plugin. |
I have no objection to this PR from a security POV. Besides that, I'm just not a maintainer of this plugin. Superficially, the change seems useful, and even looks like it improves feature parity between different config file types, but I don't know this plugin well enough to say whether it fits the overall design or direction. I have not reviewed the code other than checking it for security vulnerabilities. |
|
@olamy thanks for the approval. Any reason to keep this from getting merged? Would have been nice to see it go out with 3.7.1 which just released :( |
Indeed |
|
Waiting for tests to pass on #118 before releasing |
|
@MRamonLeon thanks! My team (and customers) are really looking forward to having this feature :) |
|
I still need to address a minor issue before merging. I hope to have it by Friday. |
|
@tykeal would you mind if I bump Jenkins core to 2.266? |
|
@MRamonLeon I have no problem with that. I had considered bumping stuff when I wrote the change, but realized that I could get everything working on the versions that were in the pom. |
With the advent of Jenkins Configuration As Code (JCasC) it has become
more needed to be able to provide a way to allow job owners to provide
new managed configuration files that also replace tokenized credentials.
While it is possible to attain this goal by utilizing the Credentials
Binding Plugin along with the 'Replace All' option when defining where
the file is put on disk during the job this has caused several problems
with well established methods of managing the jobs. In particular, jobs
being managed with Jenkins Job Builder which tend to be heavily
Freestyle in nature and also utilize a lot of templates and macros
suffer greatly when using this method of management.
Instead, allowing credentials to be defined and automatically replaced
when the file is rendered down to disk solves the problem of securely
storing the credentials in the credential store and also allowing less
privileged persons the ability to more easily work with the managed
files.
This change in particular modifies the 'custom' file type to support
defining credentials along with the tokens that are to be used and
having them properly rendered when being put down on disk. It will
continue to work fine with the previous method using the Credentials
Binding Plugin as well as it will only replace the tokens that it has a
full definition for and then would be passed through the full
environment token replacement as well as it did before this change.
Making this change allows for safe storage of configuration files in
JCasC format to be stored in a repository and in some fashion
manipulated into place for Jenkins to utilize (exercise for the admin)
which is a partial answer to JENKINS-17766. It also completely obviates
the requirement to encrypt the files as per JENKINS-18635 as the
sensitive data can now be securely separated from the non-sensitive
data.
Resolves: JENKINS-43204
Signed-off-by: Andrew Grimberg agrimberg@linuxfoundation.org