Skip to content
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

docs: document cred management with cli + specific support for ecr and google artifact registry #2077

Merged
merged 4 commits into from
Jun 1, 2024

Conversation

krancour
Copy link
Member

Fixes #2067

@krancour krancour added this to the v0.7.0 milestone May 30, 2024
@krancour krancour self-assigned this May 30, 2024
@krancour krancour requested review from a team as code owners May 30, 2024 23:02
@krancour krancour changed the title docs: documents cred management with cli + specific support for ecr and google artifact registry docs: document cred management with cli + specific support for ecr and google artifact registry May 30, 2024
Copy link

netlify bot commented May 30, 2024

Deploy Preview for docs-kargo-akuity-io ready!

Name Link
🔨 Latest commit b94cb52
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/665a617b83ecf1000875eeea
😎 Deploy Preview https://deploy-preview-2077.kargo.akuity.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented May 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.62%. Comparing base (fde59de) to head (b94cb52).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2077   +/-   ##
=======================================
  Coverage   45.62%   45.62%           
=======================================
  Files         238      238           
  Lines       16485    16485           
=======================================
  Hits         7522     7522           
  Misses       8592     8592           
  Partials      371      371           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 318 to 330
Google Artifact Registry does not _directly_ support long-lived credentials,
however, a Google service account key
[can be exchanged for a token](https://cloud.google.com/artifact-registry/docs/docker/authentication#token)
that is valid for 60 minutes. Kargo can seamlessly make this exchange and will
Copy link
Member

Choose a reason for hiding this comment

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

Google Artifact Registry does not directly support long-lived credentials,

Actually, unlike ECR, GAR can be accessed with a long-lived service account key as a password, so we should change this wording. Credentials look like:

username: _json_key_base64
password: base64 encoded of some json like:

{
  "type": "service_account",
  "project_id": "myproject",
  "private_key_id": "xxxxxx",
  "private_key": "-----BEGIN PRIVATE KEY-----\nyyyyyyy==\n-----END PRIVATE KEY-----\n",
  "client_email": "foo@myproject.iam.gserviceaccount.com",
  "client_id": "12345",
  "auth_uri": "https://accounts.google.com/o/oauth2/auth",
  "token_uri": "https://oauth2.googleapis.com/token",
  "auth_provider_x509_cert_url": "https://www.googleapis.com/oauth2/v1/certs",
  "client_x509_cert_url": "https://www.googleapis.com/robot/v1/metadata/x509/foo%40myproject.iam.gserviceaccount.com"
}

This is documented at https://cloud.google.com/artifact-registry/docs/docker/authentication#json-key,

We can point them there if they prefer/need to use long-lived credentials and explain that GAR supports two methods of accessing private repos.

TBH, I'm not sure what benefits "Access Token" auth has over "Service Account Key" since Access Token would still need a long lived service account credential anyways.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤦‍♂️ Embarrassed that I missed that part of the docs, and also question the value of access tokens under these circumstances. I will document this as a third option for authenticating to GAR. For my own edification, I might also research if/why exchanging a service account key for an access token might have any benefit over using the key directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed. Directly using the service account token is now listed as an option that works, but which Google discourages.

I have also posted a question about that recommendation on security.stackexchange.com.

https://security.stackexchange.com/questions/277163/why-use-access-token-for-google-artifact-registry-access

Copy link
Member Author

Choose a reason for hiding this comment

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

I have much more clarity about this now, and it would have been obvious if I'd re-read my own code after the inital confusion had set in...

I think I allowed the fact that _json_key_base64 and a base64-encoded service account key can be used as user/pass in basic auth to the registry to mislead me into believing that gcloud auth print-access-token (or programmatic equivalent) was using the service account key in the same way -- which is totally not the case.

The service account key is actually used to sign a JWT and it is the JWT that is exchanged for an access token -- meaning that when you request an access token, the service account key does not at any point traverse the wire.

It's right here:

config, err := google.JWTConfigFromJSON(decodedKey, "https://www.googleapis.com/auth/devstorage.read_write")
if err != nil {
return "", fmt.Errorf("error parsing service account key: %w", err)
}
tokenSource := config.TokenSource(ctx)
token, err := tokenSource.Token()
if err != nil {
return "", fmt.Errorf("error getting access token: %w", err)
}

This being the case, the benefits of obtaining an access token over using the service account key to access the registry directly are more obvious.

Two action items for me:

  1. Without going into exhaustive detail, understanding this much better now compels me to somewhat strengthen my caution against the most direct approach. I will amend this PR accordingly.

  2. While researching, I noticed that when requesting the access token, I am requesting a read/write scope, which is quite unnecessary for how Kargo interacts with a registry, which is always in a read-only fashion. I will fix this in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Docs amended in b94cb52

krancour added 2 commits May 31, 2024 13:09
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
@krancour krancour force-pushed the krancour/reg-auth-docs branch from 345610a to 6d2c186 Compare May 31, 2024 17:34
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
and transition from misleading "exchange for an access token" language
to more accurate "used to OBTAIN an access token" language

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
@krancour krancour added this pull request to the merge queue Jun 1, 2024
Merged via the queue into akuity:main with commit fc8ca7f Jun 1, 2024
17 checks passed
@krancour krancour deleted the krancour/reg-auth-docs branch June 1, 2024 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

document new image repo authn options
2 participants