fix: Updating akv cert provider to use getSecret #957

susanshi · 2023-07-31T20:36:43Z

Description

This PR includes:

Updating akv cert provider to use getSecret to fetch certificates for the entire chain
adding clean up step for dangling aks resources
Updated documentation to configure certificate policy on cert import
Uploaded existing test cert chain in e2e integration, inline root cert is no longer needed.

What this PR does / why we need it:

Fixes #695

Which issue(s) this PR fixes (optional, using `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when the PR gets merged):

Fixes #695

Type of change

Please delete options that are not relevant.

[ X] Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Helm Chart Change (any edit/addition/update that is necessary for changes merged to the main branch)
[X ] This change requires a documentation update

How Has This Been Tested?

-Uploaded existing test cert chain in e2e integration, inline root cert is no longer needed
Manual validation includes:

Secret with non content type
Secret disabled
Secret that is not a a cert
Not enough permissions for identity
PEM and PCKS12 formats

Checklist:

Does the affected code have corresponding tests?
Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
Does this introduce breaking changes that would require an announcement or bumping the major version?
Do all new files have appropriate license header?

Post Merge Requirements

MAINTAINERS: manually trigger the "Publish Package" workflow after merging any PR that indicates Helm Chart Change

codecov · 2023-08-01T04:05:38Z

Codecov Report

Patch coverage: 64.17% and project coverage change: +0.46% 🎉

Comparison is base (1051142) 56.99% compared to head (3d693c6) 57.45%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #957      +/-   ##
==========================================
+ Coverage   56.99%   57.45%   +0.46%     
==========================================
  Files          90       90              
  Lines        5292     5326      +34     
==========================================
+ Hits         3016     3060      +44     
+ Misses       1970     1956      -14     
- Partials      306      310       +4

Files Changed	Coverage Δ
pkg/certificateprovider/azurekeyvault/provider.go	`58.11% <64.17%> (+15.44%)`	⬆️

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

duffney · 2023-08-03T13:44:30Z

docs/quickstarts/ratify-on-azure.md

    ```bash
    az keyvault certificate import \
    --vault-name ${AKV_NAME} \
    -n ${KEY_NAME} \
-    -f ${CERT_PATH}


Are there any steps that walk the reader through creating a certificate? Or is that assumed knowledge or a prerequisite?

I ve actually deleted the entire kv import step, customer should follow https://learn.microsoft.com/en-us/azure/container-registry/container-registry-tutorial-sign-build-push#build-and-sign-a-container-image from the prereq section of this quick start.

scripts/azure-ci-test-cleanup.sh

test/bats/azure-test.bats

.github/workflows/build-pr.yml

scripts/azure-ci-test-cleanup.sh

pkg/certificateprovider/azurekeyvault/provider.go

binbin-li · 2023-08-07T14:58:28Z

pkg/certificateprovider/azurekeyvault/provider.go

+
+	data := []byte(*secretBundle.Value)
+
+	if *secretBundle.ContentType == PKCS12ContentType {


as mentioned in: #695 (comment), does pkcs12 works now in Golang?

Discussed offline, we will keep the current implementation but provided more guidance in documentation.

pkg/certificateprovider/azurekeyvault/provider.go

docs/quickstarts/ratify-on-azure.md

susanshi · 2023-08-08T00:29:01Z

docs/quickstarts/ratify-on-azure.md


    ```bash
    az keyvault set-policy --name ${AKV_NAME} \
-    --certificate-permissions get \
+    --secret-permissions get \


log an issue to update the ratify terraform resource

Issue logged at #973

docs/reference/oras-auth-provider.md

pkg/certificateprovider/azurekeyvault/provider.go

…to getSecretAKV2

Signed-off-by: Susan Shi <huish@microsoft.com>

susanshi · 2023-08-08T07:20:36Z

Hi @akashsinghal and @binbin-li , i ve addressed the feedback and merged with latest main. Please give another round of review, please review aks rest result and clean up log at https://github.com/susanshi/ratify/actions/runs/5793405981/job/15701535090, and

binbin-li

lgtm

akashsinghal · 2023-08-10T18:33:38Z

pkg/certificateprovider/azurekeyvault/provider.go

+				certsStatus = append(certsStatus, certProperty)
+			}
+		default:
+			logrus.Warnf("certificate '%s', version '%s': azure keyvualt certificate provider detected unknown block type %s", certName, version, block.Type)


nit: "keyvault" is mispelled in multiple places keyvualt --> keyvault

akashsinghal

LGTM

…to getSecretAKV2

Signed-off-by: Susan Shi <huish@microsoft.com>

fix: fix cert-rotator test (ratify-project#992) chore: bump github.com/aws/aws-sdk-go-v2/config from 1.18.32 to 1.18.33 (ratify-project#988) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Akash Singhal <akashsinghal@microsoft.com> feat: add graceful shutdown for http server (ratify-project#949) fix: Updating akv cert provider to use getSecret (ratify-project#957) Signed-off-by: Susan Shi <huish@microsoft.com> chore: bump golangci/golangci-lint-action from 3.6.0 to 3.7.0 (ratify-project#997) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> chore: bump actions/setup-go from 4.0.1 to 4.1.0 (ratify-project#996) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> fix: adding experimental to dynamic plugin flag (ratify-project#980) refactor: refactor error handling (ratify-project#956) docs: add notaryv2 upgrade doc (ratify-project#999) chore: update assign.yaml template chore: update library templates chore: update library templates2 chore: update library templates fix typo chore: update library templates array

Signed-off-by: Susan Shi <huish@microsoft.com>

susanshi added 2 commits July 31, 2023 20:33

updateing akv provider to use getSecret

d68d071

setting env variable

4997fbb

susanshi added 12 commits August 1, 2023 05:19

adding loop for cleanup

685aa7d

fix cleanup script

24d0449

AKVPolicy

d00e3c2

AKVPolicy Update

63a3f3b

AKVPolicy reorder

6f5804f

Add missing line continutation

e9d876f

Change contentType

9d865d0

code cleanup

84e9c91

add doc

f0cccc9

adding null check

d6bf123

fix lint

8be7414

Merge branch 'main' into getSecretAKV2

a9fb3c9

susanshi marked this pull request as ready for review August 2, 2023 06:41

susanshi requested review from akashsinghal, binbin-li, jimmyraywv, luisdlp and toddysm as code owners August 2, 2023 06:41

akashsinghal assigned susanshi Aug 2, 2023

susanshi added 3 commits August 3, 2023 06:36

handle pkcs

e07fd31

pkcs12 convert to PEM

de56c9e

update unit test

dd489b0

duffney reviewed Aug 3, 2023

View reviewed changes

susanshi added 4 commits August 4, 2023 04:29

update error msg

e98be2d

fix formatting

66e041a

error strings should not be capitalized

5b688d4

Merge branch 'main' into getSecretAKV2

95f2f6c

akashsinghal reviewed Aug 4, 2023

View reviewed changes

scripts/azure-ci-test-cleanup.sh Show resolved Hide resolved

akashsinghal reviewed Aug 4, 2023

View reviewed changes

test/bats/azure-test.bats Show resolved Hide resolved

binbin-li reviewed Aug 7, 2023

View reviewed changes

susanshi commented Aug 8, 2023

View reviewed changes

docs/quickstarts/ratify-on-azure.md Outdated Show resolved Hide resolved

susanshi commented Aug 8, 2023

View reviewed changes

docs/reference/oras-auth-provider.md Outdated Show resolved Hide resolved

susanshi commented Aug 8, 2023

View reviewed changes

pkg/certificateprovider/azurekeyvault/provider.go Show resolved Hide resolved

susanshi added 6 commits August 8, 2023 04:43

address cr feedback

5671f6e

Merge branch 'getSecretAKV2' of https://github.com/susanshi/ratify in…

235e5da

…to getSecretAKV2

Merge branch 'main' into getSecretAKV2

649e9af

Signed-off-by: Susan Shi <huish@microsoft.com>

merge with main

d5c2a93

resolve merge conflicts

aed0b5f

adding a note for inline cert

723551b

susanshi added 2 commits August 10, 2023 09:41

Merge branch 'main' into getSecretAKV2

18a38b9

Merge branch 'main' into getSecretAKV2

b943bbb

binbin-li previously approved these changes Aug 10, 2023

View reviewed changes

akashsinghal reviewed Aug 10, 2023

View reviewed changes

akashsinghal previously approved these changes Aug 10, 2023

View reviewed changes

fix typo

7aed3bf

susanshi dismissed stale reviews from akashsinghal and binbin-li via 7aed3bf August 15, 2023 01:26

susanshi added 3 commits August 15, 2023 11:31

Merge branch 'deislabs:main' into getSecretAKV2

c240685

updating crd reference doc for akv

de43fa3

Merge branch 'getSecretAKV2' of https://github.com/susanshi/ratify in…

3d693c6

…to getSecretAKV2

binbin-li approved these changes Aug 15, 2023

View reviewed changes

susanshi merged commit dcfa9b5 into ratify-project:main Aug 15, 2023
15 checks passed

junczhu pushed a commit to ZAFT-Armored-Keeper-of-Unity/ratify that referenced this pull request Aug 23, 2023

fix: Updating akv cert provider to use getSecret (ratify-project#957)

07c8fa0

Signed-off-by: Susan Shi <huish@microsoft.com>

bspaans pushed a commit to bspaans/ratify that referenced this pull request Oct 17, 2023

fix: Updating akv cert provider to use getSecret (ratify-project#957)

e5c60f0

Signed-off-by: Susan Shi <huish@microsoft.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Updating akv cert provider to use getSecret #957

fix: Updating akv cert provider to use getSecret #957

susanshi commented Jul 31, 2023 •

edited

Loading

codecov bot commented Aug 1, 2023 •

edited

Loading

duffney Aug 3, 2023

susanshi Aug 8, 2023

binbin-li Aug 7, 2023

susanshi Aug 8, 2023

susanshi Aug 8, 2023

susanshi Aug 8, 2023

susanshi commented Aug 8, 2023

binbin-li left a comment

akashsinghal Aug 10, 2023

akashsinghal left a comment


		data := []byte(*secretBundle.Value)

		if *secretBundle.ContentType == PKCS12ContentType {

fix: Updating akv cert provider to use getSecret #957

fix: Updating akv cert provider to use getSecret #957

Conversation

susanshi commented Jul 31, 2023 • edited Loading

Description

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):

Type of change

How Has This Been Tested?

Checklist:

Post Merge Requirements

codecov bot commented Aug 1, 2023 • edited Loading

Codecov Report

duffney Aug 3, 2023

Choose a reason for hiding this comment

susanshi Aug 8, 2023

Choose a reason for hiding this comment

binbin-li Aug 7, 2023

Choose a reason for hiding this comment

susanshi Aug 8, 2023

Choose a reason for hiding this comment

susanshi Aug 8, 2023

Choose a reason for hiding this comment

susanshi Aug 8, 2023

Choose a reason for hiding this comment

susanshi commented Aug 8, 2023

binbin-li left a comment

Choose a reason for hiding this comment

akashsinghal Aug 10, 2023

Choose a reason for hiding this comment

akashsinghal left a comment

Choose a reason for hiding this comment

susanshi commented Jul 31, 2023 •

edited

Loading

Which issue(s) this PR fixes (optional, using `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when the PR gets merged):

codecov bot commented Aug 1, 2023 •

edited

Loading