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

feat(cli): add trivy auth #7664

Merged
merged 20 commits into from
Oct 9, 2024
Merged

feat(cli): add trivy auth #7664

merged 20 commits into from
Oct 9, 2024

Conversation

knqyf263
Copy link
Collaborator

@knqyf263 knqyf263 commented Oct 7, 2024

Description

We have seen several use cases where people want to use Trivy in environments withoug container runtimes such as Docker. This PR adds trivy auth login and trivy auth logout to replace docker login and docker logout.

$ cat ~/my_password.txt | trivy auth login --username foo --password-stdin ghcr.io
$ trivy image ghcr.io/your/private_image
$ trivy auth logout ghcr.io

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@knqyf263 knqyf263 self-assigned this Oct 7, 2024
Signed-off-by: knqyf263 <[email protected]>
Signed-off-by: knqyf263 <[email protected]>
@knqyf263 knqyf263 marked this pull request as ready for review October 7, 2024 19:20
@knqyf263 knqyf263 requested a review from DmitriyLewen October 7, 2024 19:20
Copy link
Contributor

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

@knqyf263
left comments.
Take a loot, when you have time.

pkg/flag/registry_flags.go Outdated Show resolved Hide resolved
pkg/flag/registry_flags.go Show resolved Hide resolved
pkg/commands/auth/run.go Show resolved Hide resolved
docs/docs/references/configuration/cli/trivy_auth.md Outdated Show resolved Hide resolved
pkg/commands/auth/run.go Outdated Show resolved Hide resolved
pkg/commands/auth/run.go Show resolved Hide resolved
pkg/commands/app.go Show resolved Hide resolved
@knqyf263 knqyf263 requested a review from DmitriyLewen October 8, 2024 11:11
Copy link
Contributor

@DmitriyLewen DmitriyLewen left a comment

Choose a reason for hiding this comment

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

LGTM

@knqyf263
Copy link
Collaborator Author

knqyf263 commented Oct 9, 2024

@itaysk I'll merge this PR, but we can fix the command name anytime you find a better one.

@knqyf263 knqyf263 added this pull request to the merge queue Oct 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 9, 2024
@knqyf263 knqyf263 added this pull request to the merge queue Oct 9, 2024
github-merge-queue bot pushed a commit that referenced this pull request Oct 9, 2024
Signed-off-by: knqyf263 <[email protected]>
Co-authored-by: DmitriyLewen <[email protected]>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 9, 2024
@itaysk
Copy link
Contributor

itaysk commented Oct 9, 2024

feel free to merge, but I do think we'll need to change it.
I'll continue the discussion here (even if the PR is merged) since there's no issue/discussion.

@knqyf263 knqyf263 added this pull request to the merge queue Oct 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 9, 2024
@knqyf263 knqyf263 enabled auto-merge October 9, 2024 10:12
@knqyf263
Copy link
Collaborator Author

knqyf263 commented Oct 9, 2024

@itaysk You can open a discussion if you want. Since I didn't think we needed a discussion on this topic, and it was easy to implement, I didn't create an issue, but I'm open to discussion.

@knqyf263 knqyf263 added this pull request to the merge queue Oct 9, 2024
Merged via the queue into aquasecurity:main with commit 27117f8 Oct 9, 2024
17 checks passed
@knqyf263 knqyf263 deleted the add_auth branch October 9, 2024 10:47
@itaysk
Copy link
Contributor

itaysk commented Oct 9, 2024

I'll try to summarize my feedback into 3 points:

1 - main login command should be reserved for main api login

when we say trivy auth login it's not very clear what the purpose of this login is for. docker login, gh auth login, snyk auth, aws sso login and every other CLI tool with a login command will sign me into the main service that the CLI is representing, and allow me to interact with the service via the CLI. So as a normal user might expect that trivy auth login will allow me to sign into the trivy service - maybe this is trivy server mode, maybe this is aqua, maybe this is the future version of trivy server/aqua, but either way, I'm not sure it's expected that the main login command will authenticate me to an arbitrary container registry.

2 - registry login is not the only kind of auth we support

Flows based on container registry:

  1. pulling image to scan
  2. pulling a trivy asset from registry (except vex-db): trivy-db/trivy-java-db/trivy-checks
  3. installing wasm modules from registry

Flows not based on container registry:

  1. scanning other targets: all are authenticated ourside of trivy
    1. private repos - GitHub/GitLab token in env vars
    2. k8s clusters - kubeconfig
    3. AMI - AWS SDK env vars
    4. TF modules - TF SDK env vars
  2. vex repository: HTTP auth, or implementation specific auth
  3. trivy client/server mode: dedicated --token flag

3 - several registry authentication options

If I understand correctly, this command (or docker login) will cover all of the registry based flows listed abobe.
It appears that if the artifact is hosted in AWS/Azure/GCP registry, docker login (or this command) will not suffice, and cloud-specific authentication will be required (please correct me if wrong). Also, we currently aupport dedicated --username/--password flags for basic authentication registries.

@knqyf263
Copy link
Collaborator Author

knqyf263 commented Oct 9, 2024

docker login (or this command) will not suffice, and cloud-specific authentication will be required (please correct me if wrong)

It is required to obtain a password for docker login or trivy auth login, like aws ecr get-login-password | docker login --username AWS --password-stdin xxxxxxxxxxx.dkr.ecr.us-east-1.amazonaws.com.

trivy auth registry login appears to help some of your feedback. What do you think?

@simar7
Copy link
Member

simar7 commented Oct 9, 2024

my reservation was that when we say trivy auth login it's not very clear what the purpose of this login is for.

@itaysk that's an interesting point and to be honest I do agree, it sounds like trivy requires a login to use it's functionality.

trivy auth registry login appears to help some of your feedback. What do you think?

If we're not planning to support any other credentials in the future, trivy registry login might be shorter. But on the other hand, if we do want to support future creds (maybe telemetry?), then I agree keeping trivy auth <subcommand> <action> is probably better.

@itaysk
Copy link
Contributor

itaysk commented Oct 10, 2024

trivy auth registry login appears to help some of your feedback. What do you think?

yes it does

excuse me for thinking out loud for a bit:
we have a number of other similar commands for managing connection to external registries: trivy module, trivy plugin and trivy vex. Today these commands don't mange authentication to the respective registries (maybe in the future they will?) but I suspect that if they did, it would be under that subcommand and not under the auth subcommand. These commands are grouped as "Management commands". Based on this UX that we've evolved into, managing registry connections would have to be under: trivy registry subcommand (which is also under Management commands), i.e trivy registry login or trivy registry add).

The problem with this approach is that it might be confusing with registry scanning (which we don't support today).
Perhaps a better solution in hindsight would have been to move all those administrative commands under a subcommand trivy admin (not unlike the proposed trivy auth) including clean, module, plugin, vex, etc. that would remove any ambiguity and allow us to grow admin commands freely.

But coming back to reality, I guess trivy auth registry login is fine, and trivy registry login as well. I'd vote for the latter I think.

@itaysk
Copy link
Contributor

itaysk commented Oct 10, 2024

I worte this before reading @simar7 's response :)

@knqyf263
Copy link
Collaborator Author

I don't mind trivy auth registry login or trivy registry login. I don't think it's a good idea to nest too many commands, so trivy registry login looks slightly better.

@DmitriyLewen @nikpivkin Any thoughts? If not, I'd go for trivy registry login.

@DmitriyLewen
Copy link
Contributor

trivy registry login looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants