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

refactor: ca and cert packages #79

Merged
merged 8 commits into from
Dec 29, 2023

Conversation

mrjoelkamp
Copy link
Member

@mrjoelkamp mrjoelkamp commented Dec 21, 2023

Summary

This PR doesn't meet any goals for the smuggler interface but moves us in the right direction. The main motivation for this change is to move this common reusable CreateX509Cert() from the signed-attestation repo into openpubkey

Copy link
Contributor

@whalelines whalelines 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. Thanks for the tests. A couple questions.

ca/ca.go Outdated Show resolved Hide resolved
ca/ca_test.go Outdated Show resolved Hide resolved
ca/ca_test.go Outdated Show resolved Hide resolved
ca/ca.go Outdated Show resolved Hide resolved
Copy link
Member

@EthanHeilman EthanHeilman left a comment

Choose a reason for hiding this comment

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

I have one comment, but it isn't a blocker as the original CA didn't do any verification of the PK Token. You are welcome to address it or not.

The PR oidcClaim struct this adds will be very nice to have.

ca/ca.go Outdated Show resolved Hide resolved
@EthanHeilman
Copy link
Member

EthanHeilman commented Dec 29, 2023

Not a code review comment just curious. Is there a name for this pattern you are using here?

        type Alias OidcClaims
	aux := &struct {
		Audience any `json:"aud"`
		*Alias
	}{
		Alias: (*Alias)(id),
	}

@mrjoelkamp
Copy link
Member Author

Not a code review comment just curious. Is there a name for this pattern you are using here?

Not sure if there is a name for it but it is necessary to avoid recursive unmarshaling of OidcClaims when trying to unmarshal the aud field separately in a custom UnmarshalJSON function. By casting the receiver (id *OidcClaims) to the Alias type you can unmarshal the OidcClaims struct without calling the custom UnmarshalJSON function recursively (since it is now of type Alias instead of OidcClaims). This embeds the original struct in a new auxiliary struct that you can add additional fields to for separate handling during the unmarshaling process.

@EthanHeilman EthanHeilman merged commit c576d6b into openpubkey:main Dec 29, 2023
3 checks passed
@mrjoelkamp mrjoelkamp deleted the refactor-cert-smuggler branch January 2, 2024 14:16
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.

None yet

3 participants