Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

Use SPIFFE ID for validation if enabled on current issuing and validating MRC certificate #5160

Merged
merged 3 commits into from Nov 28, 2022
Merged

Use SPIFFE ID for validation if enabled on current issuing and validating MRC certificate #5160

merged 3 commits into from Nov 28, 2022

Conversation

jsturtevant
Copy link
Contributor

@jsturtevant jsturtevant commented Sep 28, 2022 •
edited

Signed-off-by: James Sturtevant jstur@microsoft.com

Description:
This builds on #5131 which added the SPIFFE ID into the certificates. Now that the SPIFFE ID is present we can use it to make routing and validation decisions.

Todo:

  • add e2e tests
  • add more unit tests

Fixes: #5025, #5026

Testing done:

Affected area:

Functional Area
New Functionality [x]
CI System [ ]
CLI Tool [ ]
Certificate Management [x]
Control Plane [x]
Demo [ ]
Documentation [ ]
Egress [ ]
Ingress [ ]
Install [ ]
Networking [ ]
Observability [ ]
Performance [ ]
SMI Policy [ ]
Security [ ]
Sidecar Injection [ ]
Tests [ ]
Upgrade [ ]
Other [ ]

Please answer the following questions with yes/no.

  1. Does this change contain code from or inspired by another project?

    • Did you notify the maintainers and provide attribution?
      no

  2. Is this a breaking change?
    no

  3. Has documentation corresponding to this change been updated in the osm-docs repo (if applicable)?
    no

@jsturtevant
Copy link
Contributor Author

I've opened this so can get feedback on this approach.

I considered enlightening the idenity.ServiceIdenitity to be more than a string which would allow for calls like si.AsPrincipal(b.trustDomain) to stay the same instead of giving this information to the builders to construct the appropriate rules but after trying it out this seemed like a cleaner approach and closer to what we have with the trust domain and other settings. We might consider such a change to the identity package in the future but would encompass larger changes.

@jsturtevant jsturtevant mentioned this pull request Sep 28, 2022
Closed
@jsturtevant jsturtevant force-pushed the spiffe-cert-validation branch 3 times, most recently from 025916e to 666b81e Compare October 4, 2022 17:15
jsturtevant
jsturtevant commented Oct 4, 2022
View reviewed changes
pkg/catalog/inbound_traffic_policies.go Outdated
@@ -183,7 +183,7 @@ func (mc *MeshCatalog) getRoutingRulesFromTrafficTarget(trafficTarget access.Tra
trustDomain := mc.GetTrustDomain()
allowedDownstreamPrincipals := mapset.NewSet()
for _, source := range trafficTarget.Spec.Sources {
allowedDownstreamPrincipals.Add(trafficTargetIdentityToSvcAccount(source).AsPrincipal(trustDomain))
allowedDownstreamPrincipals.Add(trafficTargetIdentityToSvcAccount(source).AsPrincipal(trustDomain, mc.SpiffEnabled()))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Expanding the principal in the mesh catalog isn't right here. This would be better to be done in the XDS builders as it is done for other XDS endpoints. This would keep the mesh catalog knowing about the OSM identities only, which is how it is done for the rest of the functions in the catalog. I will create an issue before merging to track this enhancement if agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#5236

jsturtevant
jsturtevant commented Oct 4, 2022
View reviewed changes
pkg/catalog/catalog.go Outdated Show resolved Hide resolved
@jsturtevant
Copy link
Contributor Author

on hold until #5193 merges

@jsturtevant jsturtevant mentioned this pull request Oct 11, 2022
Merged
@jsturtevant jsturtevant force-pushed the spiffe-cert-validation branch 7 times, most recently from 2655dde to 3b28547 Compare November 2, 2022 21:00
jsturtevant
jsturtevant commented Nov 2, 2022
View reviewed changes
pkg/certificate/manager.go Outdated
// Note that the CRD uses a default, so this value will always be set.
// It is up to the caller to determine if the signing and validating trust domains are different
func (m *Manager) GetTrustDomains() TrustDomain {
func (m *Manager) GetIssuers() IssuerInfo {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept this as a struct since it keeps it on the stack vs allocating a list in the heap based on #5193 (comment)

@jsturtevant jsturtevant mentioned this pull request Nov 2, 2022
Closed
@jsturtevant
Copy link
Contributor Author

@openservicemesh/osm-maintainers this is ready for review 🚀

@jsturtevant jsturtevant force-pushed the spiffe-cert-validation branch 2 times, most recently from 6c7c88d to 24cbc32 Compare November 10, 2022 21:34
@jsturtevant
Use SPIFFE ID for validation if enabled on active certificate
c14fed6 
Signed-off-by: James Sturtevant <jstur@microsoft.com>
@jsturtevant
Copy link
Contributor Author

rebased on all the latest changes

jaellio
jaellio reviewed Nov 23, 2022
View reviewed changes
pkg/certificate/manager.go Outdated Show resolved Hide resolved
pkg/certificate/manager.go Outdated Show resolved Hide resolved
pkg/certificate/manager.go Outdated Show resolved Hide resolved
pkg/certificate/manager_test.go Outdated Show resolved Hide resolved
pkg/certificate/manager_test.go Show resolved Hide resolved
pkg/envoy/generator/sds/builder_test.go Outdated Show resolved Hide resolved
pkg/identity/types.go Show resolved Hide resolved
pkg/osm/callbacks.go Outdated Show resolved Hide resolved
@jsturtevant
Respond to feedback
f3687da 
Signed-off-by: James Sturtevant <jstur@microsoft.com>
@jsturtevant
Add test back
131b056 
Signed-off-by: James Sturtevant <jstur@microsoft.com>
@keithmattix keithmattix merged commit 2e98052 into openservicemesh:main Nov 28, 2022
@jsturtevant jsturtevant mentioned this pull request Nov 29, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@keithmattix keithmattix keithmattix approved these changes

@jaellio jaellio jaellio approved these changes

@shashankram shashankram Awaiting requested review from shashankram

@snehachhabria snehachhabria Awaiting requested review from snehachhabria

@nojnhuh nojnhuh Awaiting requested review from nojnhuh

@draychev draychev Awaiting requested review from draychev

@trstringer trstringer Awaiting requested review from trstringer

@steeling steeling Awaiting requested review from steeling

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement spiffeid x509 validation for Certifices
3 participants
@jsturtevant @keithmattix @jaellio