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

core: SpiffeUtil API for extracting Spiffe URI and loading TrustBundles #11575

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

erm-g
Copy link
Contributor

@erm-g erm-g commented Sep 30, 2024

Additional API for SpiffeUtil:

  • extract Spiffe URI from certificate chain
  • load Spiffe Trust Bundle from filesystem json spec JWK spec

@erm-g erm-g mentioned this pull request Sep 30, 2024
core/src/main/java/io/grpc/internal/JsonParser.java Outdated Show resolved Hide resolved
core/src/main/java/io/grpc/internal/JsonParser.java Outdated Show resolved Hide resolved
core/src/main/java/io/grpc/internal/SpiffeUtil.java Outdated Show resolved Hide resolved
core/src/main/java/io/grpc/internal/SpiffeUtil.java Outdated Show resolved Hide resolved
core/src/main/java/io/grpc/internal/SpiffeUtil.java Outdated Show resolved Hide resolved
cronet/build.gradle Outdated Show resolved Hide resolved
Map<String, Long> sequenceNumbers = new HashMap<>();
for (String trustDomainName : trustDomainsNode.keySet()) {
Map<String, ?> domainNode = JsonUtil.getObject(trustDomainsNode, trustDomainName);
if (domainNode == null || domainNode.size() == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

null is impossible, because that means the entry doesn't exist.

Copy link
Member

Choose a reason for hiding this comment

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

Ditto for kty and use.

public void loadTrustBundleFromFileFailureTest() throws Exception {
// Check the exception if JSON root element is different from 'trust_domains'
NullPointerException npe = assertThrows(NullPointerException.class, () -> SpiffeUtil
.loadTrustBundleFromFile(Paths.get(ClassLoader.getSystemResource(TEST_DIRECTORY_PREFIX
Copy link
Member

@ejona86 ejona86 Oct 4, 2024

Choose a reason for hiding this comment

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

This is depending on specifics of the test environment that aren't guaranteed. In particular, with Blaze/Bazel this won't be the case. The necessary approach is to read the resource as a stream and copy it to a temporary file.

I was going to comment on the API that it would be better to pass an InputStream instead of a filename (but decided I didn't care). Things like this become much easier that way (no need to make a temporary file).

@@ -56,7 +58,8 @@ public static Object parse(String raw) throws IOException {
}
}

private static Object parseRecursive(JsonReader jr) throws IOException {
private static Object parseRecursive(JsonReader jr)
throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

Unwrap. Ditto below.

jr.beginObject();
Map<String, Object> obj = new LinkedHashMap<>();
while (jr.hasNext()) {
String name = jr.nextName();
checkArgument(!obj.containsKey(name), "Duplicate key found: " + name);
Copy link
Member

Choose a reason for hiding this comment

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

This allocates the error string even when there isn't an error. Use built-in formatting available to checkArgument.

Map<String, Long> sequenceNumbers = new HashMap<>();
for (String trustDomainName : trustDomainsNode.keySet()) {
Map<String, ?> domainNode = JsonUtil.getObject(trustDomainsNode, trustDomainName);
if (domainNode == null || domainNode.size() == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto for kty and use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants