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.

Sign up for GitHub

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

Jump to bottom

s2a: Address minor comments on PR#11113 #11540

Merged
merged 11 commits into from
Sep 27, 2024
Merged

s2a: Address minor comments on PR#11113 #11540

merged 11 commits into from
Sep 27, 2024

Conversation

rmehta19
Copy link
Contributor

Address minor comments on #11113

@rmehta19 rmehta19 mentioned this pull request Sep 20, 2024
Merged
@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Sep 23, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Sep 23, 2024
@rmehta19
Copy link
Contributor Author

Hi @ejona86 & @larry-safran, are there any additional comments for me to address on this PR?

ejona86
ejona86 reviewed Sep 27, 2024
View reviewed changes
s2a/src/test/java/io/grpc/s2a/handshaker/FakeWriter.java

// src/test/resources/leaf_key_ec.pem
Copy link
Member

Choose a reason for hiding this comment

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

What's special about this one that it isn't being read from the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same key as in leaf_key_ec.pem, it just has newlines removed.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how that answers my question. It is a literal string to avoid removing the newlines? string.replaceAll("\n", "")

Looks like it also has the header/footer removed. Is io.grpc.util.CertificateUtils.getPrivateKey() what you need?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, done in 48e8c7f

s2a/src/test/resources/int_cert1_.cnf
prompt = no

[req_distinguished_name]
O = o
Copy link
Member

Choose a reason for hiding this comment

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

Every level has the same subject name? That just seems like we hate our future selves if we ever have to debug. Let's make them unique in some way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in a48dc83

ejona86
ejona86 approved these changes Sep 27, 2024
View reviewed changes
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

I'm fine with getting this in and comments being resolved afterward. It is strictly an improvement.

@ejona86 ejona86 merged commit fa18fec into grpc:master Sep 27, 2024
17 checks passed
This was referenced Oct 1, 2024
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ejona86 ejona86 ejona86 approved these changes

@larry-safran larry-safran larry-safran approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rmehta19 @ejona86 @larry-safran @grpc-kokoro