Bugfix: Removed @RequirePost and changed ssh key migration behavior

[djesionek]

I changed the behavior from my feature added in #445 as suggested by @daniel-beck by moving the migration code to EC2Cloud.readResolve(). I also tested again the migration from 1.51 to 1.52. I could confirm that it wasn't working before and that it works now for me with that change.
Not sure why the @RequirePost lead to issues as it was requested as an addition but maybe someone can clarify if there is a better way than just removing it for now.

Thanks to @varyvol for the bugnotice and @daniel-beck for the hint how to fix it quickly.

[MRamonLeon]

It looks like fine to remove this annotation here, the method is just returning whether the credential-id showed is an actual rsa private key. But better to confirm with some security person: @daniel-beck, @Wadeck, ...

[daniel-beck]

There's still a permission check.

The docs for this are at https://www.jenkins.io/doc/developer/security/form-validation/ -- please read and if anything specific is unclear, ask.

[Wadeck]

getSshCredential retrieves credentials, but as the RequirePOST protects against CSRF, there is no issue. The method is already checking for ADMINISTER.

Side note, such methods should not have troubles to use POST for the check. You can change the

ec2-plugin/src/main/resources/hudson/plugins/ec2/AmazonEC2Cloud/config-entries.jelly

Line 39 in 8702cc4

<f:entry field="sshKeysCredentialsId" title="${%EC2 Key Pair's Private Key}">

(and similar for Eucalyptus) by adding checkMethod="post"

[Wadeck]

@markyjackson-taulia Please read the comment above about the RequirePOST ;)

A @RequirePOST is required when doing doCheckX
(from #445 (review))

---

Not sure why the @RequirePost lead to issues as it was requested as an addition

Be sure to test your code before commiting, there is no "manual test" performed everytime on PR and this could lead to broken release sometimes ;)

[djesionek]

Now I tested the migration manually after all changes maybe we should release that as a hotfix or something before more updates fail.

[varyvol]

@MRamonLeon @res0nance @daniel-beck Tests are passing and even if no test was created to verify the issue, @djesionek has tested it manually. Should we maybe release this and work in the test later?

[res0nance]

I think releasing as a bugfix release is reasonable. waiting to see if there are any other comments

[batmat]

I think that if really we've had people manually verifying the fix, we may want to merge, then do a followup PR for an automated test.

However, given the relative ease of use of @LocalData, I would recommend we also have a test accompanying this fix so it's caught next time.

• Docs: https://javadoc.jenkins.io/component/jenkins-test-harness/org/jvnet/hudson/test/recipes/LocalData.html
• Working example: https://github.com/batmat/localdata-jenkinsrule/

[varyvol]

I've been trying to create an automated test with @LocalData but I've not been able so far: the clouds are not loaded by Jenkins.

But I've manually verified this is working properly: the credential is created and assigned to the existing cloud and the verification for the SSH key works now.

[varyvol]

See #500 for an automated test.