Skip to content

feat: store authentication identifier in attestation #2139

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jun 30, 2025
Merged

feat: store authentication identifier in attestation #2139

merged 7 commits into from
Jun 30, 2025

Conversation

Piskoo
Copy link
Collaborator

@Piskoo Piskoo commented Jun 26, 2025

Closes #2127

Adds authentication details (type + id) to attestations, supporting:

  • User Token
   "predicate": {
      "auth": {
         "id": "34030166-826c-4c72-a635-353230fedb47",
         "type": "user"
      },
  • Api Token
   "predicate": {
      "auth": {
         "id": "342be6de-e469-45c6-99c1-5b886b206d57",
         "type": "api-token"
      },

More tokens can be supported with changes to ParseToken function.

@Piskoo
feat(attestation): store authentication identifier in attestation
67a4865 
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
@Piskoo Piskoo marked this pull request as draft June 26, 2025 14:28
Piskoo added 2 commits June 26, 2025 17:03
@Piskoo
fix: lint errors
d2dcb7e 
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
@Piskoo
fix: lint errors
dba70ba 
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
@Piskoo Piskoo marked this pull request as ready for review June 26, 2025 15:14
migmartri
migmartri reviewed Jun 26, 2025
View reviewed changes
app/cli/pkg/token.go
// See the License for the specific language governing permissions and
// limitations under the License.

package token
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is not how packages work in golang, everything in pkg must be the same package, so with your setup, you can only have token in the pkg/ directory, is that what you wanted?

If that's not what you wanted, I'd suggest to do

cli/internal/token/token.go, internal so it "indicates" it's not for sharing with other apps, and token/ to create the actual package namespace. Does it make sense?

Copy link
Member

Choose a reason for hiding this comment

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

btw, just FYI, I find this reference guide quite handy https://go.dev/doc/effective_go#package-names

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it, thanks!

migmartri
migmartri reviewed Jun 26, 2025
View reviewed changes
Copy link
Member

@migmartri migmartri left a comment

Choose a reason for hiding this comment

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

Very cool! Just take a look at my comment about organizing packages and we are good to go!

app/cli/cmd/root.go Outdated
// For telemetry reasons we parse the token to know the type of token is being used when executing the CLI
// Once we have the token type we can send it to the telemetry service by injecting it on the context
token, err := parseToken(token)
authToken, err := token.ParseToken(authToken)
Copy link
Member

Choose a reason for hiding this comment

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

we could prevent stuttering of the function name by calling it just Parse. so token.Parse(...) should be enough :)

pkg/attestation/crafter/api/attestation/v1/crafting_state.proto Outdated

message Auth {
// Type of authentication used (USER, API_TOKEN, FEDERATED_GITLAB_TOKEN)
string type = 1 [(buf.validate.field).string.min_len = 1];
Copy link
Member

Choose a reason for hiding this comment

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

maybe you can add an enum here

Piskoo added 4 commits June 27, 2025 09:05
@Piskoo
rename parse token function
624ced3 
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
@Piskoo
rename add auth type enum
ec0a482 
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
@Piskoo
move token package to cli internal
dc26c5e 
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
@Piskoo
fix crafting state indentation
9936e6a 
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
migmartri
migmartri reviewed Jun 27, 2025
View reviewed changes
pkg/attestation/crafter/api/attestation/v1/crafting_state.proto
enum AuthType {
AUTH_TYPE_UNSPECIFIED = 0;
AUTH_TYPE_USER = 1;
AUTH_TYPE_API_TOKEN = 2;
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to also add AUTH_TYPE_FEDERATED and store in the ID maybe the issuer

that way we could know that's a FEDERATED token issued by github.com.

See below an example like this one (note, it's expired :)

eyJraWQiOiI0aTNzRkU3c3hxTlBPVDdGZHZjR0ExWlZHR0lfci10c0RYbkV1WVQ0WnFFIiwidHlwIjoiSldUIiwiYWxnIjoiUlMyNTYifQ.eyJuYW1lc3BhY2VfaWQiOiI2MDM2NTYyMCIsIm5hbWVzcGFjZV9wYXRoIjoibWlnbWFydHJpMSIsInByb2plY3RfaWQiOiI0MTA5MzMzMiIsInByb2plY3RfcGF0aCI6Im1pZ21hcnRyaTEvdGVzdCIsInVzZXJfaWQiOiIxMzA1NTQyMyIsInVzZXJfbG9naW4iOiJtaWdtYXJ0cmkiLCJ1c2VyX2VtYWlsIjoibWlnbWFydHJpQGdtYWlsLmNvbSIsInVzZXJfYWNjZXNzX2xldmVsIjoib3duZXIiLCJwaXBlbGluZV9pZCI6IjE4OTMzNjM2NTkiLCJwaXBlbGluZV9zb3VyY2UiOiJwdXNoIiwiam9iX2lkIjoiMTA0ODk0Nzk2MDMiLCJyZWYiOiJtYWluIiwicmVmX3R5cGUiOiJicmFuY2giLCJyZWZfcGF0aCI6InJlZnMvaGVhZHMvbWFpbiIsInJlZl9wcm90ZWN0ZWQiOiJ0cnVlIiwicnVubmVyX2lkIjoxMjI3MDgwNywicnVubmVyX2Vudmlyb25tZW50IjoiZ2l0bGFiLWhvc3RlZCIsInNoYSI6ImYwYWI3OTEwY2MzMmM4YmI1MDYwMjY4YmE2MjNlOGE0YjUwN2U1MjciLCJwcm9qZWN0X3Zpc2liaWxpdHkiOiJwcml2YXRlIiwiY2lfY29uZmlnX3JlZl91cmkiOiJnaXRsYWIuY29tL21pZ21hcnRyaTEvdGVzdC8vLmdpdGxhYi1jaS55bWxAcmVmcy9oZWFkcy9tYWluIiwiY2lfY29uZmlnX3NoYSI6ImYwYWI3OTEwY2MzMmM4YmI1MDYwMjY4YmE2MjNlOGE0YjUwN2U1MjciLCJqdGkiOiI0ZTBkZDJhNy1jOGRiLTQ3OTYtYmFkYi05MjlhYzBiZjQyNGMiLCJpYXQiOjE3NTEwMTA0MjUsIm5iZiI6MTc1MTAxMDQyMCwiZXhwIjoxNzUxMDE0MDI1LCJpc3MiOiJodHRwczovL2dpdGxhYi5jb20iLCJzdWIiOiJwcm9qZWN0X3BhdGg6bWlnbWFydHJpMS90ZXN0OnJlZl90eXBlOmJyYW5jaDpyZWY6bWFpbiIsImF1ZCI6ImNoYWlubG9vcCJ9.lu6-4-BVXuqQXMF-Kkfqhb7ZCi-ktR6tJkLEHYFY2WYQ_GmtjWWo8UjMd6VMwUBu_gCE90ZoHQIAJuY720MacR45__-zGgT20ZLKOAku1iha6j_F3oJz0Iz0P1IOFFAOG-di39NnRZjkUgqsC0arc85L508CF1adcE_knDUvvo4S7zw3_nn0bJRtIAMl_NDSIP7GjtkQhqjLsx2adE1NIV930DnB9IAT6_T8wzag4BJsiXsOZkY3si5QRoE59EbhvhxTi_nAj0zDCB1Z82-KxhLL-SapkFHTvdl2Ie50K0gox1cjBbee7KhccSzLFfTdTewxnI5ulho3KbL7UUwd2g

jiparis
jiparis approved these changes Jun 27, 2025
View reviewed changes
Copy link
Member

@jiparis jiparis left a comment

Choose a reason for hiding this comment

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

It's looking great. Thanks @Piskoo
Regarding the federated token, I think we can leave to a follow up PR.

app/cli/internal/token/token.go
apiTokenAudience = "api-token-auth.chainloop"
)

// Parse the token and return the type of token. At the moment in Chainloop we have 3 types of tokens:
Copy link
Member

Choose a reason for hiding this comment

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

If we add the Federated one, we will have 3 types and this sentence will be true 😆

@Piskoo Piskoo merged commit d89ac24 into chainloop-dev:main Jun 30, 2025
13 checks passed
@migmartri
Copy link
Member

It's looking great. Thanks @Piskoo Regarding the federated token, I think we can leave to a follow up PR.

let's not forget to add this, it's important for SLSA

@Piskoo Piskoo mentioned this pull request Jun 30, 2025
Merged
@BrewTestBot BrewTestBot mentioned this pull request Jun 30, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@migmartri migmartri migmartri left review comments

@jiparis jiparis jiparis approved these changes

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

Successfully merging this pull request may close these issues.

feat: store authentication identifier in attestation
3 participants
@Piskoo @migmartri @jiparis