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: add self_signed pluginConfig #119

Merged
merged 7 commits into from Jul 18, 2023

Conversation

JeyJeyGao
Copy link
Collaborator

@JeyJeyGao JeyJeyGao commented Jul 13, 2023

Feat:

  • added self_signed parameter in the pluginConfig for self-signed certificate (Certificates Get permission)

Fix:

  • removed as_secert pluginConfig
  • secerts get is the default behavior when self_signed and ca_certs are not set (Secrets Get permission)
  • added a message to remind user to enable secrets get permission
  • raise exception when the ca_certs is empty

Parameters logic:

  • self_signed is true && ca_certs=/path/to/cacerts ==> exception
  • self_signed is true && ca_certs is null or empty ==> get self-signed certificate with Certificates Get permission
  • self_signed is false or not set && ca_certs=/path/to/cacerts ==> get leaf certificate with Certificates Get permission and read the certificate bundle from file
  • self_signed is false or not set && ca_certs is not set or empty ==> get certificate chain with Secrets Get permission

Signed-off-by: Junjie Gao junjiegao@microsoft.com

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #119 (6550dd7) into main (6bf62ca) will increase coverage by 0.22%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #119      +/-   ##
==========================================
+ Coverage   93.97%   94.19%   +0.22%     
==========================================
  Files          15       15              
  Lines         448      465      +17     
==========================================
+ Hits          421      438      +17     
  Misses         27       27              
Impacted Files Coverage Δ
...gin.AzureKeyVault/Certificate/CertificateBundle.cs 100.00% <100.00%> (ø)
....Plugin.AzureKeyVault/Command/GenerateSignature.cs 100.00% <100.00%> (ø)

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
@JeyJeyGao JeyJeyGao changed the title fix: remove as_secret config feat: add self_signed pluginConfig Jul 14, 2023
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Copy link
Member

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM with suggestions

docs/self-signed-workflow.md Outdated Show resolved Hide resolved
docs/self-signed-workflow.md Outdated Show resolved Hide resolved
docs/self-signed-workflow.md Outdated Show resolved Hide resolved
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Copy link
Member

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@Two-Hearts Two-Hearts left a comment

Choose a reason for hiding this comment

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

I highly recommend that we document this behavior somewhere in this Repo for user's reference, otherwise the learning experience for new users gonna be a real pain :)

Parameters logic:
self_signed is true && ca_certs=/path/to/cacerts ==> exception
self_signed is true && ca_certs is null or empty ==> get self-signed certificate with Certificates Get permission
self_signed is false or not set && ca_certs=/path/to/cacerts ==> get leaf certificate with Certificates Get permission and read the certificate bundle from file
self_signed is false or not set && ca_certs is not set or empty ==> get certificate chain with Secrets Get permission

/cc: @shizhMSFT @yizha1 @FeynmanZhou

@JeyJeyGao JeyJeyGao merged commit 38aac67 into Azure:main Jul 18, 2023
5 checks passed
@JeyJeyGao
Copy link
Collaborator Author

I highly recommend that we document this behavior somewhere in this Repo for user's reference, otherwise the learning experience for new users gonna be a real pain :)

Parameters logic:
self_signed is true && ca_certs=/path/to/cacerts ==> exception
self_signed is true && ca_certs is null or empty ==> get self-signed certificate with Certificates Get permission
self_signed is false or not set && ca_certs=/path/to/cacerts ==> get leaf certificate with Certificates Get permission and read the certificate bundle from file
self_signed is false or not set && ca_certs is not set or empty ==> get certificate chain with Secrets Get permission

/cc: @shizhMSFT @yizha1 @FeynmanZhou

I will create a new RR to update the documentations and include your suggestion.

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.

None yet

3 participants