Skip to content

added import comments #654

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.

Sign up for GitHub

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

Closed
daniel-sanche wants to merge 1 commit into from
Closed

added import comments #654

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 26 additions & 4 deletions kms/asymmetric/samples.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
// Samples for asymmetric keys feature of Cloud Key Management Service: https://cloud.google.com/kms/
package samples

// [START kms_get_asymmetric_public]

import (
"context"
"crypto"
Expand All @@ -25,11 +23,13 @@ import (
"google.golang.org/api/cloudkms/v1"
)

// [END kms_get_asymmetric_public]

// [START kms_get_asymmetric_public]

// getAsymmetricPublicKey retrieves the public key from a saved asymmetric key pair on KMS.
//
// Requires:
Copy link
Contributor

@tbpg tbpg Oct 19, 2018

Choose a reason for hiding this comment

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

These lines are a little bit weird since they don't list every import, only some of them. Would you mind splitting all of these functions into separate files and include the import() block in the region tag? I think that's the pattern the canonical snippet will follow, so it would be a step in the right direction.

Copy link
Member Author

@daniel-sanche daniel-sanche Oct 19, 2018

Choose a reason for hiding this comment

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

I've had conversations with Kurtis (GoogleCloudPlatform/java-docs-samples#1218) and the product team (b/113025156) on how best to handle this. We decided that until we have an agreed upon standard way of handling imports, it would be best to leave the imports in the comments for now. This gives the users the information they need, and it's easy to be consistent with how we present the information across all supported languages without significant refactors.

As for not including all imports, I assumed standard imports like fmt, errors, and context would be considered a given for anyone familiar with go, but I can add them if you think it would be useful

Copy link
Contributor

@tbpg tbpg Oct 19, 2018

Choose a reason for hiding this comment

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

I'm not sure. These are all standard library imports. So, goimports should find them. So, I'm not sure this is worth adding.

If we decide on the right way to include imports in the canonical snippet, we can update this.

Does that make sense?

Copy link
Member Author

@daniel-sanche daniel-sanche Oct 19, 2018

Choose a reason for hiding this comment

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

I suppose. I'd prefer to include these for consistency sake since these comments are needed for python and java. Even in go it might not be clear whether we're using standard crypto libraries or 3rd party ones with similar names, so I'd prefer to be explicit. But I guess these aren't as necessary for go

I am going to want to remove the region tags from the import block though, so this PR should stay open either way

// crypto/x509
// encoding/pem
func getAsymmetricPublicKey(ctx context.Context, client *cloudkms.Service, keyPath string) (interface{}, error) {
response, err := client.Projects.Locations.KeyRings.CryptoKeys.CryptoKeyVersions.
GetPublicKey(keyPath).Context(ctx).Do()
Expand All @@ -50,6 +50,9 @@ func getAsymmetricPublicKey(ctx context.Context, client *cloudkms.Service, keyPa
// [START kms_decrypt_rsa]

// decryptRSA will attempt to decrypt a given ciphertext with an 'RSA_DECRYPT_OAEP_2048_SHA256' private key.stored on Cloud KMS
//
// Requires:
// encoding/base64
func decryptRSA(ctx context.Context, client *cloudkms.Service, keyPath string, ciphertext []byte) ([]byte, error) {
decryptRequest := &cloudkms.AsymmetricDecryptRequest{
Ciphertext: base64.StdEncoding.EncodeToString(ciphertext),
Expand All @@ -72,6 +75,11 @@ func decryptRSA(ctx context.Context, client *cloudkms.Service, keyPath string, c
// [START kms_encrypt_rsa]

// encryptRSA will encrypt data locally using an 'RSA_DECRYPT_OAEP_2048_SHA256' public key retrieved from Cloud KMS
//
// Requires:
// crypto/rand
// crypto/rsa
// crypto/sha256
func encryptRSA(ctx context.Context, client *cloudkms.Service, keyPath string, plaintext []byte) ([]byte, error) {
abstractKey, err := getAsymmetricPublicKey(ctx, client, keyPath)
if err != nil {
Expand All @@ -93,6 +101,10 @@ func encryptRSA(ctx context.Context, client *cloudkms.Service, keyPath string, p
// [START kms_sign_asymmetric]

// signAsymmetric will sign a plaintext message using a saved asymmetric private key.
//
// Requires:
// crypto/sha256
// encoding/base64
func signAsymmetric(ctx context.Context, client *cloudkms.Service, keyPath string, message []byte) ([]byte, error) {
// Note: some key algorithms will require a different hash function.
// For example, EC_SIGN_P384_SHA384 requires SHA-384.
Expand Down Expand Up @@ -120,6 +132,10 @@ func signAsymmetric(ctx context.Context, client *cloudkms.Service, keyPath strin
// [START kms_verify_signature_rsa]

// verifySignatureRSA will verify that an 'RSA_SIGN_PSS_2048_SHA256' signature is valid for a given message.
//
// Requires:
// crypto/rsa
// crypto/sha256
func verifySignatureRSA(ctx context.Context, client *cloudkms.Service, keyPath string, signature, message []byte) error {
abstractKey, err := getAsymmetricPublicKey(ctx, client, keyPath)
if err != nil {
Expand All @@ -144,6 +160,12 @@ func verifySignatureRSA(ctx context.Context, client *cloudkms.Service, keyPath s
// [START kms_verify_signature_ec]

// verifySignatureEC will verify that an 'EC_SIGN_P256_SHA256' signature is valid for a given message.
//
// Requires:
// crypto/ecdsa
// crypto/sha256
// encoding/asn1
// math/big
func verifySignatureEC(ctx context.Context, client *cloudkms.Service, keyPath string, signature, message []byte) error {
abstractKey, err := getAsymmetricPublicKey(ctx, client, keyPath)
if err != nil {
Expand Down