Skip to content

Conversation

myidpt
Copy link

@myidpt myidpt commented Jul 14, 2020

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.

@myidpt myidpt requested review from costinm and rshriram July 14, 2020 00:02
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jul 14, 2020
@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 14, 2020
@costinm
Copy link
Contributor

costinm commented Jul 14, 2020

We have the problem that now istio-agent has 2 similar but incompatible protos - Istiod and Google Mesh CA.
This PR is neither - so now we have 3 protos we need to deal with, and a migration.

I personally don't care if we adopt Google Mesh CA proto or Istiod - except the names they do the same thing.
But please - pick one and make an exact copy. Yes, the proto in istiod is alpha - but that's fine, new APIs should
not be beta and it's not even clear there is consensus on this as a long-term protocol ( vs. ACME and other options ).

Main difference is the package name - which unfortunately is significant in gRPC, it's part of the URL. One is
istio.v1.auth
the other is
google.security.meshca.v1
and your PR has
istio.security.v1beta1
There seem to also be proto numbers changed. Removing unused fields is fine.

So if community is ok with having 'google' in the service name - I would use that and we'll have a single gRPC.
Otherwise - let's keep the EXACT proto, since there is no RFC proposing changes to what we have - and the migration
planning, testing, etc associated.

Copy link
Contributor

@costinm costinm left a 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 ?

@rshriram
Copy link
Member

if we are going to make changes to what exists, then I would like to add the bootstrap node metadata here as well.

@howardjohn
Copy link
Member

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

Copy link
Contributor

@costinm costinm left a 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.

Copy link
Contributor

@costinm costinm left a 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. "

@myidpt myidpt requested a review from a team as a code owner July 14, 2020 21:54
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 14, 2020
@myidpt
Copy link
Author

myidpt commented Jul 14, 2020

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.
Now it's backward compatible.
Updated the comments as well.

@myidpt
Copy link
Author

myidpt commented Jul 14, 2020

/test build_api

@myidpt myidpt requested a review from howardjohn July 14, 2020 23:21
@istio-testing istio-testing merged commit ada27df into istio:master Jul 14, 2020
@linsun
Copy link
Member

linsun commented Jul 15, 2020

@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.

myidpt pushed a commit to myidpt/api that referenced this pull request Jul 21, 2020
This is to remove the wrongly generated files in
istio#1520.
istio-testing pushed a commit that referenced this pull request Jul 21, 2020
This is to remove the wrongly generated files in
#1520.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants