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

Offer a way to start the RealJenkinsRule instance with https only #858

Merged
merged 31 commits into from
Oct 10, 2024

Conversation

Vlatombe
Copy link
Member

@Vlatombe Vlatombe commented Oct 4, 2024

This allows to start RealJenkinsRule instances listening on https instead of http.
A self-signed certificate matching the given host name (or localhost if unspecified) is automatically generated (using bouncycastle)

Various methods are offered to allow callers to access the RealJenkinsRule clients to access the instance without disabling security.

Also added utilities related to key store and self-signed certificate management.

Testing done

Submitter checklist

  • 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

@Vlatombe Vlatombe requested a review from jglick October 9, 2024 15:15
@Vlatombe Vlatombe marked this pull request as ready for review October 9, 2024 15:23
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
/**
* Creates a new instance using the default keystore type.
* @param path path of the keystore file. If it exists, it will be loaded automatically.
* @param password password for the keystore file.
Copy link
Member

Choose a reason for hiding this comment

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

Can this be optional, since for this kind of test we do not care about keystore passwords?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not for now; jenkinsci/winstone#417

@jglick jglick requested a review from jtnord October 9, 2024 18:50
@jtnord
Copy link
Member

jtnord commented Oct 9, 2024

A self-signed certificate matching the given host name (or localhost if unspecified) is automatically generated (using bouncycastle)

is there a reason you need to access the RealJenkinsRule via the host name? if testing multiple RealJenkinsRules side by side and you want a nice jenkins-1 using some form of nip.io then would it not behove us to add a static SSL cert generated once and checked in (and skip generating on demand) that has subject altanmes for
localhost, ::1, 127.0.0.1, *.nip.io, *.localhost.me ..,.. (insert any other providers here).

@Vlatombe
Copy link
Member Author

is there a reason you need to access the RealJenkinsRule via the host name?

The point here is to test close to real life scenarios:

  • a reverse proxy set up with https (public-jenkins.acme.com)
  • a RealJenkinsRule instance set up with https (internal-jenkins.acme.com)

Each one using different https certificates (so external users must trust CA for the reverse proxy, then reverse proxy must trust CA from RJR instance).

Checking in a static certificate would do if I had only one RealJenkinsRule instance, but I want guarantee that trust stores are setup appropriately so for test purpose, I think it is better to have a single certificate generated for each instance.

@jtnord
Copy link
Member

jtnord commented Oct 10, 2024

  • a reverse proxy set up with https (public-jenkins.acme.com)
  • a RealJenkinsRule instance set up with https (internal-jenkins.acme.com)

This seems like a very specific setup useful for a very narrow set of plugins (one or two?).
Check-in the 3 certs in the specific project(s) and expose a method to use a specific cert (or keystore) for a specific RealJenkinsRule instance. Then the default checked in cert here work all the other plugins that just want a valid TLS endpoint.

or employ shading in this project so that the library does not leak and will not affect plugins using the bouncycastle-api either regularly or in the PCT.

}

private static void log(JenkinsRule r) throws IOException {
LOGGER.info("Running on " + r.getURL().toExternalForm());
Copy link
Member

Choose a reason for hiding this comment

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

Could strengthen this slightly to return the URL and show that it is https.

Copy link
Member Author

Choose a reason for hiding this comment

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

Prints something like

[INFO] [stdout] 2024-10-10 12:53:16.687+0000 [id=94]    INFO    o.j.h.t.RealJenkinsRuleHttpsTest#log: Running on https://localhost:54912/jenkins/

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I just meant that the test JVM could assert that.

var options = InboundAgentRule.Options
.newBuilder()
.name("remote")
.webSocket()
Copy link
Member

Choose a reason for hiding this comment

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

I guess either way this exercises the changes, since even a TCP agent would make an initial HTTP handshake by default. (InboundAgentRule does not support direct mode AFAIK.)

Copy link
Member Author

Choose a reason for hiding this comment

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

TCP listener needs to be enabled explicitly on the controller side (otherwise the client gets <url> appears to be publishing an invalid X-Instance-Identity.) so I just went for the simplest option.

Co-authored-by: Jesse Glick <jglick@cloudbees.com>
subjectKeyIdentifier = hash

[ alternate_names ]
DNS.1 = localhost
Copy link
Member

Choose a reason for hiding this comment

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

I thought we would include at least *.localtest.me?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not needed for now.

pom.xml Outdated Show resolved Hide resolved
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Looks good AFAICT

@jglick jglick requested a review from jtnord October 10, 2024 12:47
pom.xml Outdated Show resolved Hide resolved
@Vlatombe Vlatombe merged commit 35346d9 into jenkinsci:master Oct 10, 2024
14 of 15 checks passed
@Vlatombe Vlatombe deleted the https branch October 10, 2024 14:33
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