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

Split sagoodkey.NewKeyPolicy from goodkey.NewKeyPolicy #6651

Merged
merged 2 commits into from
Feb 13, 2023

Conversation

mtrmac
Copy link
Contributor

@mtrmac mtrmac commented Feb 10, 2023

... so that goodkey no longer depends on google.golang.org/grpc and github.com/letsencrypt/boulder/sa/proto , making it cheaper to use from external Go code.

For better or worse, the goodkey package is used by external callers like https://github.com/sigstore/sigstore/blob/6ba2c727c278dd46958e3446c5f8039d00a27308/pkg/cryptoutils/publickey.go#L148 ; those callers have no use for the GRPC definitions.

This PR allows such callers to no longer include the github.com/letsencrypt/boulder/core/proto and github.com/letsencrypt/boulder/sa/proto subpackages, allowing to shave 295 kB from the binary size.

Existing tests were used unchanged in the new sagoodkey package, and adapted directly in goodkey.

@mtrmac mtrmac requested a review from a team as a code owner February 10, 2023 21:30
... so that goodkey no longer depends on google.golang.org/grpc and
github.com/letsencrypt/boulder/sa/proto , making it cheaper to use
from external Go code.

Signed-off-by: Miloslav Trmač <[email protected]>
aarongable
aarongable previously approved these changes Feb 11, 2023
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

Hey, this is cool, thank you! This change seems like the cleanest way I can think of to achieve this separation. LGTM with one minor nit, and we require two approvals on all changes here so I'm requesting review from the rest of the team as well.

@@ -146,10 +146,10 @@ func (policy *KeyPolicy) GoodKey(ctx context.Context, key crypto.PublicKey) erro
if err != nil {
return badKey("%w", err)
}
exists, err := policy.dbCheck(ctx, &sapb.KeyBlockedRequest{KeyHash: digest[:]})
exists, err := policy.dbCheck(ctx, digest[:])
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that BlockedKeyCheckFunc is much more generic, I'd consider renaming policy.dbCheck to policy.blockedCheck or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, thanks!

@aarongable aarongable requested a review from a team February 11, 2023 00:55
@aarongable aarongable requested a review from a team February 13, 2023 17:24
@beautifulentropy beautifulentropy merged commit 5daf7d9 into letsencrypt:main Feb 13, 2023
@mtrmac
Copy link
Contributor Author

mtrmac commented Feb 13, 2023

Thanks!

@mtrmac mtrmac deleted the goodkey-external-use branch February 13, 2023 21:36
mtrmac added a commit to mtrmac/image that referenced this pull request Feb 13, 2023
This removes quite a bit of dependent code (295 kB on macOS).

It also adds a new github.com/go-jose/go-jose.v2 , but we'll
get rid of that again after letsencrypt/boulder#6581
lands.

Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac added a commit to mtrmac/image that referenced this pull request Feb 14, 2023
This removes quite a bit of dependent code (295 kB on macOS).

It also adds a new github.com/go-jose/go-jose.v2 , but we'll
get rid of that again after letsencrypt/boulder#6581
lands.

Signed-off-by: Miloslav Trmač <[email protected]>
rhatdan added a commit to containers/image that referenced this pull request Feb 14, 2023
jonjohnsonjr added a commit to jonjohnsonjr/sigstore that referenced this pull request May 18, 2023
This picks up letsencrypt/boulder#6651 which
makes it easier to untangle our dependency on grpc and protobuf.
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