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: implement get-plugin-metadata & describe-key command in dotnet #89

Merged
merged 10 commits into from
Apr 20, 2023

Conversation

JeyJeyGao
Copy link
Collaborator

@JeyJeyGao JeyJeyGao commented Apr 18, 2023

Resolve part of #90
Signed-off-by: Junjie Gao [email protected]

@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Merging #89 (3b4176b) into main (12bb0ad) will not change coverage.
The diff coverage is n/a.

❗ Current head 3b4176b differs from pull request most recent head 75c01bc. Consider uploading reports for the commit 75c01bc to get more accurate results

@@           Coverage Diff           @@
##             main      #89   +/-   ##
=======================================
  Coverage   69.60%   69.60%           
=======================================
  Files           6        6           
  Lines         306      306           
=======================================
  Hits          213      213           
  Misses         87       87           
  Partials        6        6           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

akv-plugin/cmd/describe-key.cs Outdated Show resolved Hide resolved
akv-plugin/cmd/get-plugin-metadata.cs Outdated Show resolved Hide resolved
akv-plugin/proto/get-plugin-metadata.cs Outdated Show resolved Hide resolved
akv-plugin/Program.cs Outdated Show resolved Hide resolved
akv-plugin/proto/key.cs Outdated Show resolved Hide resolved
akv-plugin/proto/io.cs Outdated Show resolved Hide resolved
akv-plugin/proto/io.cs Outdated Show resolved Hide resolved
Comment on lines 42 to 68
RSA? rsaKey = signingCert.GetRSAPublicKey();
ECDsa? ecdsaKey = signingCert.GetECDsaPublicKey();

if (rsaKey != null)
{
int bitSize = rsaKey.KeySize;

if (bitSize == 2048 || bitSize == 3072 || bitSize == 4096)
{
return new KeySpec("RSA", bitSize);
}

throw new ValidationException($"RSA key size {bitSize} bits is not supported");
}
else if (ecdsaKey != null)
{
int bitSize = ecdsaKey.KeySize;

if (bitSize == 256 || bitSize == 384 || bitSize == 521)
{
return new KeySpec("EC", bitSize);
}

throw new ValidationException($"ECDSA key size {bitSize} bits is not supported");
}

throw new ValidationException("Unsupported public key type");
Copy link
Member

Choose a reason for hiding this comment

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

Can we have other better way to determine the public key?

Copy link
Collaborator Author

@JeyJeyGao JeyJeyGao Apr 19, 2023

Choose a reason for hiding this comment

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

We can get the key by signingCert.PublicKey.Key but PublicKey.Key is obsolete. It asks me to use the appropriate method to get the public key, such as GetRSAPublicKey.

So I optimized the code based on current logic.

@JeyJeyGao JeyJeyGao force-pushed the feat/dotnet_plugin branch from 41bb4de to c687536 Compare April 19, 2023 02:35
Signed-off-by: Junjie Gao <[email protected]>
Signed-off-by: Junjie Gao <[email protected]>
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.

It would better if we have more meaningful documentation and references of protocols.

Notation.Plugin.AzureKeyVault/cmd/DescribeKey.cs Outdated Show resolved Hide resolved
Notation.Plugin.AzureKeyVault/proto/DescribeKey.cs Outdated Show resolved Hide resolved
Signed-off-by: Junjie Gao <[email protected]>
Signed-off-by: Junjie Gao <[email protected]>
Signed-off-by: Junjie Gao <[email protected]>
Signed-off-by: Junjie Gao <[email protected]>
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

@shizhMSFT shizhMSFT merged commit f0a552f into Azure:main Apr 20, 2023
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.

2 participants