Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-70000] Fix HTTPS user cert auth for (temporary) private CA #120

Merged
merged 17 commits into from
Jul 1, 2023

Conversation

jimklimov
Copy link
Contributor

@jimklimov jimklimov commented Nov 2, 2022

https://issues.jenkins.io/browse/JENKINS-70000

Our tests are done against SUTs which provide their newly made CA which issues both web-server and user (login) certificates. As such, we can not use JDK or Jenkins persistent cert db files to trust those.

Jenkins Certificate credentials can actually store certificates (not just user keys as commonly used), but http-request-plugin can not build the trust chain for such almost-self-signed certs.

It took me several days to track down what went wrong in the web of KeyStore and Cert processing implementations involved, but ultimately I've reproduced and fixed the issue. Lack of trust handling in the plugin required relatively little changes (after all most time spent was in the and docs and other components' sources). A much larger effort was to make at least an internal test case which uploads the certs and keys involved, so I could step though code as I iterated this change and collected roadblocks.

When generating the P12 keystore to upload, one can add private CA certificates as Trusted - it does not suffice to just have a chain (attached to private key) ending with that cert. One notable caveat here is that keystore password must be nontrivial (not null or empty string; however a single space is good enough), otherwise PKCS12KeyStore ignores certs in keystore.

While I have locally made the test cases for this, they do not make much sense for the internet publication (involving private CA for temporary servers on LAN). I can try to adapt that code for publication, if someone can walk me through spawning an HTTPS server in Java, with access protected by user certs (something for the test to query with and without certs for a meaningful activity). FWIW, creation of in-memory PKCS12 keystore from PEM file(s) with keys and certificates seems easy with code examples like https://stackoverflow.com/a/48173910/4715872 and https://stackoverflow.com/a/25650305/4715872 - just keystore.store() not into a file, but into a byte array stream which then is fed to UploadedKeyStoreSource. Otherwise someone turning https://github.com/ctron/pem-keystore into a Jenkins plugin (or extending the standard one to accept more keystore file types) sounds like an option.

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

…redentials with trusted certs (private CA)

* try to loadTrustMaterial(),
* warn if keystore pass is trivial so PKCS12 does not load everything,
* support ignoreSslErrors flag,
* and log progress through auth()
@jimklimov jimklimov requested a review from a team as a code owner November 2, 2022 21:26
@jimklimov
Copy link
Contributor Author

jimklimov commented Nov 23, 2022

Conjured up some unit tests to make sure new functionality gets called.

Historically, I had local-branch tests for this complete with parsing PEM data of our local SUT certs and constructing a credential on the fly, making a separate-JVM agent, and whatnot - too much (and partially too intimate) to publish. Then my focus shifted to jenkinsci/credentials-plugin#391 as a related problem (JENKINS-70101), and much of the test suite was cleaned up and published there. And finally a copy drifted back here to publishable branch, along with "credentials-plugin"'s approach of pre-populated P12 keystore files among test resources.

After that other issue (JENKINS-70101) is addressed, the tests here may get expanded with a remote-agent case.

Also note that these tests DO NOT make actual HTTPS queries offsite, less so to some usercert-protected resource. That would be a very different effort to even set up such an instance (especially if self-contained in the test suite). The tests added now focus on plugin handling of the credential to extract Trusted and Key "Materials" while preparing the request, and not crashing overall.

Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good to me and is well supported by tests.

@jimklimov
Copy link
Contributor Author

...and? Can it be merged then? :-D

@dcoraboeuf
Copy link

Hi,

Is there any hope to have this merged & released? Or is there a workaround to get custom certificates used by the httpRequest plugin?

Thanks,
Damien

@jimklimov jimklimov force-pushed the fix-cert-auth branch 2 times, most recently from 2937c95 to cc65bcf Compare June 26, 2023 15:34
@jimklimov
Copy link
Contributor Author

Bumped to reconcile README changes in master branch.

@MarkEWaite MarkEWaite merged commit 6a3df15 into jenkinsci:master Jul 1, 2023
@jimklimov
Copy link
Contributor Author

Yay! Thanks :)

Now we can worry about one less custom-forked plugin :)

@jimklimov jimklimov deleted the fix-cert-auth branch July 2, 2023 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants