-
Notifications
You must be signed in to change notification settings - Fork 576
Make cert signing API public in istio/api. #1520
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
Conversation
We have the problem that now istio-agent has 2 similar but incompatible protos - Istiod and Google Mesh CA. I personally don't care if we adopt Google Mesh CA proto or Istiod - except the names they do the same thing. Main difference is the package name - which unfortunately is significant in gRPC, it's part of the URL. One is So if community is ok with having 'google' in the service name - I would use that and we'll have a single gRPC. |
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.
for example validity is int64 in the existing protocol - and csr has proto number 2 in google proto.
I think safest is to just make the exact copy of istio proto. All the migration pain is not worth it for replacing an int64 with a proto.
Doesn't the CSR include not before/after - i.e. validity ? Maybe we can just not include it in the proto ?
if we are going to make changes to what exists, then I would like to add the bootstrap node metadata here as well. |
I agree with everything costin said so I won't repeat it. If we are going to invent a 3rd API, which I don't think we should we at the very least need a clear upgrade story which we don't have |
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.
Thanks.
To be honest: I don't mind if you remove the validity and subject, easy to add later if needed and doesn't affect compatibility. My point was to be compatible with current Istiod and agent - i.e. package name and the csr protocol number.
But keeping as is works too.
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.
Thanks.
To be honest: I don't mind if you remove the validity and subject, easy to add later if needed and doesn't affect compatibility. My point was to be compatible with current Istiod and agent - i.e. package name and the csr protocol number.
But keeping as is works too.
One thing we must add: please document the magic header used to pass the JWT token, and I believe there was another header added by Lin for multicluster support. Both are still part of the protocol.
I would also add a section "The service implementation is REQUIRED to verify the authenticated caller is authorize to all SANs in the CSR. Service may modify the list of SANs or validity based on its policy. "
I moved this to security/v1alpha1, since the compiler doesn't allow protos in different packages (ca.proto is using package istio.v1.auth, VS istio.security.v1beta1) to be in the same directory. |
/test build_api |
@hzxuzhonghu @irisdingbj do you recall the header you added for multicluster CSR? I recall it is to help identify the caller (which cluster) who sends the CSR to istiod. |
This is to remove the wrongly generated files in istio#1520.
This is to remove the wrongly generated files in #1520.
istio/istio#24553
This copies https://github.com/istio/istio/blob/master/security/proto/istioca.proto with ambiguous field "subject_id" removed.
It offers the public API for custom agents or CAs to integration with Istio. This proto is open to reasonable extensions for adoption of more CAs. All the extensions must be approved by the Istio security WG.
Will make changes on istio-agent and Citadel in istio/istio to support this API.