-
Notifications
You must be signed in to change notification settings - Fork 11
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
Define Certificate message type for Kingdom internal services. #78
Conversation
This includes some fields denormalized from the X.509 certificate so they can be queried.
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.
Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)
src/main/proto/wfa/measurement/internal/kingdom/certificate.proto, line 43 at r1 (raw file):
// RFC 5280 revocation state. enum RevocationState { REVOCATION_STATE_UNSPECIFIED = 0;
How are valid Certs represented? Using REVOCATION_STATE_UNSPECIFIED
? Is it worthwhile to add a specific state for this?
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @efoxepstein)
src/main/proto/wfa/measurement/internal/kingdom/certificate.proto, line 43 at r1 (raw file):
Previously, efoxepstein (Eli Fox-Epstein) wrote…
How are valid Certs represented? Using
REVOCATION_STATE_UNSPECIFIED
? Is it worthwhile to add a specific state for this?
A valid cert is one that has both no revocation state specified and the current time is within its validity period.
If this enum was representing the overall certificate state, we'd indeed want to require that a state value is specified and the _UNSPECIFIED
value would be considered an error. This is instead something that provides details about revocation from RFC 5280.
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.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @corbantek, @oliver-amzn, @SanjayVas, and @zachcwc)
src/main/proto/wfa/measurement/internal/kingdom/certificate.proto, line 25 at r1 (raw file):
message Certificate { oneof parent {
You'll move to strings later?
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @corbantek, @kungfucraig, @oliver-amzn, and @zachcwc)
src/main/proto/wfa/measurement/internal/kingdom/certificate.proto, line 25 at r1 (raw file):
Previously, kungfucraig (Craig Wright) wrote…
You'll move to strings later?
You're thinking of the switch to resource names in the public API. This PR is for internal services where we don't need to be quite so generic.
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @corbantek, @kungfucraig, @oliver-amzn, and @zachcwc)
src/main/proto/wfa/measurement/internal/kingdom/certificate.proto, line 25 at r1 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
You're thinking of the switch to resource names in the public API. This PR is for internal services where we don't need to be quite so generic.
For reference, the other change I mentioned is being handled in WIP PR world-federation-of-advertisers/cross-media-measurement-api#39.
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.
Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kungfucraig, @oliver-amzn, and @zachcwc)
This includes some fields denormalized from the X.509 certificate so they can be queried.
For issue #52.
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)