-
Notifications
You must be signed in to change notification settings - Fork 5
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
Support adding CA Certificates #1
Conversation
Generate changelog in
|
'printCaTruststoreAliases', | ||
// TODO: avoid resolving from root configuration somehow, removed in Gradle 8 | ||
'--warning-mode=none') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like we may not need to suppress warnings here either?
'printCaTruststoreAliases', | |
// TODO: avoid resolving from root configuration somehow, removed in Gradle 8 | |
'--warning-mode=none') | |
'printCaTruststoreAliases') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻 yep, will fix it up with the conflicts
|
||
jdkSpec.caCerts().caCerts().forEach((name, caCertFile) -> { | ||
addCaCert(javaHome, name, caCertFile); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, given two projects which use azul-17 x86_64, what happens if both projects specify completely different certificates? Do we need to include the certificates in our cache key? I'm not sure that we should mutate the jvm in a potentially leaky way such that tests pass on project B only if you've run project A first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the CA certificates configurable at the root level/all jdks level, so all the jdks will have the same CA certs. misread what you wrote
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certificates are included in the cache key:
gradle-jdks/gradle-jdks/src/main/java/com/palantir/gradle/jdks/JdkSpec.java
Lines 35 to 40 in 8c10819
String.join("\n", "Distribution: %s", "Version: %s", "Os: %s", "Arch: %s", "CaCerts: %s"), | |
distributionName().uiName(), | |
release().version(), | |
release().os(), | |
release().arch(), | |
caCerts().combinedInSortedOrder()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah fantastic, I missed that in the block above :-)
@@ -31,6 +32,8 @@ public abstract class JdksExtension { | |||
|
|||
protected abstract MapProperty<JavaLanguageVersion, JdkExtension> getJdks(); | |||
|
|||
public abstract MapProperty<String, File> getCaCerts(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be cleaner to take PEM formatted CA certs as strings, that way we don't need to think about file I/O. iirc they can be fed to keytool via stdin when no -file
is specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, nice spot! I did this initially but figured keytool just always needs a file
gradle-jdks/src/main/java/com/palantir/gradle/jdks/CaCerts.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Before this PR
There's no way to get a CA certificate installed into the downloaded JDK's
lib/security/cacerts
root truststore. For companies that use TLS interception internally this means any test that reaches out to the internet will fail.Going from people having their own JDKs with this manually added to our JDKs they have no/little control over means this could be a real sticking point basically immediately.
After this PR
==COMMIT_MSG==
Provide a way to add CA certs to all installed JDKs.
==COMMIT_MSG==
Possible downsides?