[JENKINS-56927] Implementation of credentials for private SSH keys used by EC2 plugin #445
Conversation
…key is prevented to be showed in CasC export
|
Hi @jvz and @res0nance , |
jvz
requested review from
jvz,
res0nance,
MRamonLeon,
daniel-beck,
Wadeck and
jeffret-b
|
Ooh, this looks nice! I've added some people as reviewers to look through this. |
|
(I will not have the time to review it, sorry) |
src/main/resources/hudson/plugins/ec2/AmazonEC2Cloud/config-entries.jelly
Outdated
Show resolved
Hide resolved
src/main/resources/hudson/plugins/ec2/Eucalyptus/config-entries.jelly
Outdated
Show resolved
Hide resolved
|
What is missing on this PR to be merged? @djesionek @markyjackson-taulia |
|
AFAIK I adressed all the requested changes from the reviewers that reviewed this PR. I'm just waiting for either feedback or a merge. |
Well, I have just seen that at least what you can see below, there is a conflict on the EC2Cloud.java file |
|
Thanks for the info, that one seems to be new. I won't be able to resolve it until Sunday probably. |
|
Can someone tell me what's missing to get the reviews processed? |
|
@djesionek FYI in https://issues.jenkins-ci.org/browse/SECURITY-1883 @daniel-beck asked @jetersen to have a look into this. |
|
@froblesmartin thanks for the info somehow my jenkins community account got lost that's probably why I missed that. |
|
not sure what I can assist with. |
|
And I'm not sure what is missing for getting this merged other than maybe further reviews when necessary. Maybe @daniel-beck knows more? |
|
This plugin needs active maintainers. I've re-added the |
@jvz I have been thinking since a while about maintaining a plugin to start contributing. This could be a perfect moment 😋 . Following https://www.jenkins.io/doc/developer/plugin-governance/adopt-a-plugin/, I am going to send an email to the DL requesting access 👍 |
|
Thanks, hope it gets released soon 😄 |
|
@res0nance @MRamonLeon @fcojfernandez @froblesmartin could you please consider doing a release for this? Thanks! |
|
How was migration tested here? I had one EC2 cloud configured with 1.50.3 and when updated to 1.52, the cloud is there but no credentials are in place for the private key. Also an error is displayed because the "doCheckSshKeysCredentialsId" requires POST and the validation performed when the credential is selected is not using that. |
| this.sshKeysCredentialsId = sshKeysCredentialsId; | ||
|
|
||
| if (this.sshKeysCredentialsId == null && ( this.privateKey != null || privateKey != null)){ | ||
| migratePrivateSshKeyToCredential(this.privateKey != null ? this.privateKey.getPrivateKey() : privateKey); |
There was a problem hiding this comment.
A migration call in the constructor makes no sense. This should be in readResolve. This was probably never tested with existing stored data.
|
I made a PR with a fix for that and retested the migration scenario there manually. Maybe it would be good to create an automated test for that but at this point I have no clue how this could be achieved 😄 |
|
This broke our setup as the old key wasn't migrated. |
|
ec2-1.53 fixes it. |
This should fix the issue described in JENKINS-56927.
I tried to keep as much of the credential logic itself contained inside the EC2Cloud class but still in a way that the private key itself is not being stored longer than necessary outside of the credential API.
This implementation also takes care of migrating the old plaintext ssh configuration to the credentials API. This was checked in a test environment and also the unit tests themselves are not modified more than necessary and some of them still relied on the old storage method and a migration is performed in each one.
In my tests the privateKey was still included in plaintext in the CasC yaml when exporting, so I managed to find a quite hacky solution for that by returning null in the getter and creating a static method resolving the credential of a EC2Cloud object to a EC2PrivateKey which is now used instead in all places. If there is a better way for that I have not found it yet...
All of this introduced one new dependency: org.jenkins-ci.plugins:ssh-credentials:1.14
As the aws-credentials plugin has a different dependency to the credentials package it also was fixed to be min version 2.1.17 to prevent dependency errors. I have not checked how low we could go here but I'm also not sure how important this might be here.
Eucalyptus implementations also were updated accordingly with this but could not be tested explicitly due to lack of a test environment on my side.