-
Notifications
You must be signed in to change notification settings - Fork 4.4k
VAULT-31185 & 31186/use identity token auth for Artifactory in Vault CE & Ent #31255
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
base: main
Are you sure you want to change the base?
Conversation
c73a9a0
to
5b0c872
Compare
CI Results: |
Build Results: |
@@ -44,8 +44,7 @@ jobs: | |||
ENOS_VAR_vault_log_level: trace | |||
ENOS_VAR_aws_ssh_private_key_path: ./support/private_key.pem | |||
ENOS_VAR_tfc_api_token: ${{ secrets.TF_API_TOKEN }} | |||
ENOS_VAR_artifactory_username: ${{ secrets.ARTIFACTORY_USER }} | |||
ENOS_VAR_artifactory_token: ${{ secrets.ARTIFACTORY_TOKEN }} | |||
ENOS_VAR_artifactory_token: ${{ secrets.ARTIFACTORY_BEARER_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.
@ryancragun should this still be secrets.
or something else?
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 would say that it is only this for historical reasons. When it was initially written we used Github Actions secrets for everything. Now we prefer Github Actions secrets for public repos and the internal Vault for private repos.
So to answer: ideally we'd only have this secret set in hashicorp/vault
and only use it when the workflow executes in the context of that repository. When it executes in hashicorp/vault-enterprise
we probably ought to be retrieving it from Vault.
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.
The way that we do it in test-run-enos-scenario-matrix
maps exactly to how we ought to do it here.
6543d34
to
1f75fcc
Compare
1f75fcc
to
59d026a
Compare
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.
This looks good. Lets clean up the changelog and do some manual verification of the dev scenarios and merge it!
changelog/31255.txt
Outdated
@@ -0,0 +1,3 @@ | |||
```release-note:change | |||
secrets/artifactory: Use Artifactory identity token for auth, instead of username with API key |
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.
Since we're only changing the behavior of Vault CI, not the Vault binary itself, we don't need a changelog. Instead, use the pr/no-changelog
label to get rid of failed check.
@@ -44,8 +44,7 @@ jobs: | |||
ENOS_VAR_vault_log_level: trace | |||
ENOS_VAR_aws_ssh_private_key_path: ./support/private_key.pem | |||
ENOS_VAR_tfc_api_token: ${{ secrets.TF_API_TOKEN }} | |||
ENOS_VAR_artifactory_username: ${{ secrets.ARTIFACTORY_USER }} | |||
ENOS_VAR_artifactory_token: ${{ secrets.ARTIFACTORY_TOKEN }} | |||
ENOS_VAR_artifactory_token: ${{ secrets.ARTIFACTORY_BEARER_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.
The way that we do it in test-run-enos-scenario-matrix
maps exactly to how we ought to do it here.
artifactory_token: | ||
The artifactory token associated with your username. You'll need this if you wish to use | ||
deb or rpm artifacts! You can create a token by logging into Artifactory via Okta. | ||
The artifactory identity token associated with a service account. You'll need this if you wish to use |
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.
nit: can we normalize the line length on these new changes?
@@ -1,13 +1,6 @@ | |||
// Copyright (c) HashiCorp, Inc. | |||
// SPDX-License-Identifier: BUSL-1.1 | |||
|
|||
variable "artifactory_username" { |
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.
🫗 for basic auth
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.
🪦
@@ -73,12 +68,11 @@ module "artifact_metadata" { | |||
} | |||
|
|||
data "enos_artifactory_item" "vault" { | |||
username = var.artifactory_username |
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.
Before merge we probably ought to test the dev scenarios with the package:rpm
and package:deb
variants to make sure they're working as expected.
I added the backport labels. Even though release testing always happens from |
561a4dc
to
e02156c
Compare
e02156c
to
4e85cd0
Compare
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
ddc315a
to
dc8bbbc
Compare
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.
Lets update the descriptions in both dev scenarios and then merge!
dc8bbbc
to
e8efa40
Compare
e8efa40
to
7b5e964
Compare
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.
Description
What does this PR do?
Context: Artifactory username/ API keys are being deprecated in favor of identity tokens, which do not require a username.
TODO only if you're a HashiCorp employee
backport/
label that matches the desired release branch. Note that in the CE repo, the latest release branch will look likebackport/x.x.x
, but older release branches will bebackport/ent/x.x.x+ent
.of a public function, even if that change is in a CE file, double check that
applying the patch for this PR to the ENT repo and running tests doesn't
break any tests. Sometimes ENT only tests rely on public functions in CE
files.
in the PR description, commit message, or branch name.
description. Also, make sure the changelog is in this PR, not in your ENT PR.
PCI review checklist
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.