-
Notifications
You must be signed in to change notification settings - Fork 38
feat(attestation): federated verification #1825
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
feat(attestation): federated verification #1825
Conversation
Signed-off-by: Miguel Martinez <miguel@chainloop.dev>
Signed-off-by: Miguel Martinez <miguel@chainloop.dev>
app/controlplane/internal/conf/controlplane/config/v1/conf.proto
Outdated
Show resolved
Hide resolved
for _, tc := range testCases { | ||
t.Run(tc.name, func(t *testing.T) { | ||
ctx := transport.NewServerContext(context.Background(), &mockTransport{reqHeader: tc.tokenHeader}) | ||
logger := log.NewStdLogger(io.Discard) |
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.
Is this a leftover?
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.
No, a logger is now required by the use case
app/controlplane/internal/conf/controlplane/config/v1/conf.proto
Outdated
Show resolved
Hide resolved
return nil, fmt.Errorf("failed to read response body: %w", err) | ||
} | ||
|
||
var response struct { |
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.
We might want to make this struct public (in pkg), so that implementors can use it
} | ||
|
||
// If federated verification is enabled, we try to get the information remotely | ||
if o.federatedVerificationURL != "" { |
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.
I might be missing where the decision is made, but Is this logic called always, no matter if the token belongs to the federated provider?
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.
That's correct, the federated provider is the one that will figure out if it understands the provided token
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.
Cannot this be a performance issue? Doesn't it have an issuer
claim that can be used to early detect if the token is federated or not?
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.
It could, but note that this happens after the fact all the other checks have been completed. There is also a cache in place.
What kind of performance issue can you foresee?
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.
Calling an external URL in every single request to check a token that might be already been processed and understood. Unless I'm missing something, of course.
Signed-off-by: Miguel Martinez <miguel@chainloop.dev>
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.
LGTM!
This patch adds support for delegated, federated verification during the attestation process.
This means that when an API token is received during attestation, now, in addition to the built-in chainloop authenticator, we can delegate the authN/authZ to a third party service that we already trust.
This feature is configurable via the Helm Chart
Demo: Attestation process using a Gitlab token, there is no difference :)
You can also provide the --org to the att command if needed
Closes #1817