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.

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

fix(KeyStoreAdmin): Exceptions for Mutations when KMS Key is Disabled #1235

Open
wants to merge 2 commits into
base: mutations/mutations
Choose a base branch
from
+54 −7
Open

fix(KeyStoreAdmin): Exceptions for Mutations when KMS Key is Disabled #1235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b94627
fix(Mutations): KeyStoreAdmin Exceptions for Mutations when KMS Key i…
texastony Jan 21, 2025
f180e85
fix(Mutations): KMS Error while generating at init is MutationFrom
texastony Jan 31, 2025
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
6 changes: 3 additions & 3 deletions ...ryptographicMaterialProviders/dafny/AwsCryptographyKeyStore/src/KMSKeystoreOperations.dfy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ module {:options "/functionSyntax:4" } KMSKeystoreOperations {
grantTokens: KMS.GrantTokenList,
kmsClient: KMS.IKMSClient
)
returns (res: Result<KMS.GenerateDataKeyWithoutPlaintextResponse, Types.Error>)
returns (res: Result<KMS.GenerateDataKeyWithoutPlaintextResponse, KmsError>)
requires kmsClient.ValidState()
Comment on lines +105 to 106
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, this is a breaking change right?

Copy link
Contributor

Choose a reason for hiding this comment

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

No. This is a nice way to do it. KmsError is a sub set type. So it is only some types of Types.Error. This is a good mechanism to make sure that an operation only returns a specific set of errors.

requires HasKeyId(kmsConfiguration) && KmsArn.ValidKmsArn?(GetKeyId(kmsConfiguration))
requires AttemptKmsOperation?(kmsConfiguration, encryptionContext)
Expand Down Expand Up @@ -145,14 +145,14 @@ module {:options "/functionSyntax:4" } KMSKeystoreOperations {

:- Need(
&& generateResponse.KeyId.Some?,
Types.KeyStoreException(
Types.KeyManagementException(
message := "Invalid response from KMS GenerateDataKey:: Invalid Key Id")
);
Comment on lines 149 to 150
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: We should make both these messages consistent.

Suggested change
message := "Invalid response from KMS GenerateDataKey:: Invalid Key Id")
);
message := "Invalid response from AWS KMS GenerateDataKey:: Invalid Key Id")
);


:- Need(
&& generateResponse.CiphertextBlob.Some?
&& KMS.IsValid_CiphertextType(generateResponse.CiphertextBlob.value),
Types.KeyStoreException(
Types.KeyManagementException(
message := "Invalid response from AWS KMS GenerateDataKey: Invalid ciphertext")
);

Expand Down
14 changes: 11 additions & 3 deletions ...ptographicMaterialProviders/dafny/AwsCryptographyKeyStoreAdmin/src/InitializeMutation.dfy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ include "KmsUtils.dfy"
include "MutationIndexUtils.dfy"
include "SystemKey/Handler.dfy"
include "Mutations.dfy"
include "MutationErrorRefinement.dfy"

module {:options "/functionSyntax:4" } InternalInitializeMutation {
// StandardLibrary Imports
Expand All @@ -33,6 +34,7 @@ module {:options "/functionSyntax:4" } InternalInitializeMutation {
import MutationIndexUtils
import SystemKeyHandler = SystemKey.Handler
import Mutations
import MutationErrorRefinement

datatype InternalInitializeMutationInput = | InternalInitializeMutationInput (
nameonly Identifier: string ,
Expand Down Expand Up @@ -417,12 +419,18 @@ module {:options "/functionSyntax:4" } InternalInitializeMutation {
grantTokens := grantTokens,
kmsClient := kmsClient
);
var wrappedDecryptOnlyBranchKey :- wrappedDecryptOnlyBranchKey?
.MapFailure(e => Types.Error.AwsCryptographyKeyStore(e));

if (wrappedDecryptOnlyBranchKey?.Failure?) {
var error := MutationErrorRefinement.GenerateNewActiveException(
identifier := decryptOnlyEncryptionContext[Structure.BRANCH_KEY_IDENTIFIER_FIELD],
kmsArn := mutationToApply.Terminal.kmsArn,
error := wrappedDecryptOnlyBranchKey?.error);
return Failure(error);
}

var newDecryptOnly := Structure.ConstructEncryptedHierarchicalKey(
decryptOnlyEncryptionContext,
wrappedDecryptOnlyBranchKey.CiphertextBlob.value
wrappedDecryptOnlyBranchKey?.value.CiphertextBlob.value
);

:- Need(
Expand Down
41 changes: 40 additions & 1 deletion ...aphicMaterialProviders/dafny/AwsCryptographyKeyStoreAdmin/src/MutationErrorRefinement.dfy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,29 @@ module {:options "/functionSyntax:4" } MutationErrorRefinement {
+ "\nKMS Message: " + errorMessage?.UnwrapOr("")
}

function GenerateNewActiveException(
nameonly identifier: string,
nameonly kmsArn: string,
nameonly error: KMSKeystoreOperations.KmsError,
nameonly localOperation: string := "InitializeMutation",
nameonly kmsOperation: string := "GenerateDataKeyWithoutPlaintext"
): (output: Types.Error)
{
var opaqueKmsError? := KmsUtils.ExtractKmsOpaque(error);
var kmsErrorMessage? := KmsUtils.ExtractMessageFromKmsError(error);
var errorContext := ParsedErrorContext(
localOperation := localOperation,
kmsOperation := kmsOperation,
identifier := identifier,
itemType := Structure.BRANCH_KEY_ACTIVE_TYPE,
errorMessage? := kmsErrorMessage?);
var message :=
"Key Management denied access while creating the new Active item."
+ " Mutation is halted. Check access to KMS ARN: " + kmsArn + " ."
+ "\n" + errorContext;
Types.MutationToException(message := message)
}

function CreateActiveException(
nameonly branchKeyItem: KeyStoreTypes.EncryptedHierarchicalKey,
nameonly error: KMSKeystoreOperations.KmsError,
Expand Down Expand Up @@ -166,7 +189,23 @@ module {:options "/functionSyntax:4" } MutationErrorRefinement {
message := "Key Management through an exception."
+ " Mutation is halted. Check access to KMS."
+ "\n" + errorContext);

}
}
if (kmsWithMsg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Both this and the last case in the if (kmsWithMsg && knownKmsStrat) { block (which never happens) indicate that the statement (kmsWithMsg && knownKmsStrat) { is always true so long as kmsWithMessage is true. We return some exception in each case. Are you sure that the error messages won't overlap? For instance, this line:

var hasEncrypt? := String.HasSubString(kmsErrorMessage?.value, "Encrypt");
Wouldn't this interfere with:

else if (hasTerminalArn?.Some?) {
        return Types.MutationToException(
            message := "Key Management denied access to the terminal KMS Key."
            + " Mutation is halted. Check encrypt access to KMS ARN: " + terminalKmsArn + "."
            + "\n" + errorContext
          );

Example -- If the KMS Message has hasTerminalArn? as true, and the message says you don't have Encrypt permissions, wouldn't this be returned in

case "Decrypt/Encrypt" =>

rather than

else if (hasTerminalArn?.Some?) {

I am also concerned that if all the if blocks in all the cases in

match kmsOperation {

are exhaustive, Line 194:

    if (kmsWithMsg) {

will never be triggered because in the previous if block (kmsWithMsg && knownKmsStrat) {, knownKmsStrat is always true.

var hasOriginalArn? := String.HasSubString(kmsErrorMessage?.value, item.KmsArn);
var hasTerminalArn? := String.HasSubString(kmsErrorMessage?.value, terminalKmsArn);
if (hasOriginalArn?.Some?) {
return Types.MutationFromException(
message := "Key Management denied access to the original KMS Key."
+ " Mutation is halted. Check access to KMS ARN: " + item.KmsArn + "."
+ "\n" + errorContext
);
} else if (hasTerminalArn?.Some?) {
return Types.MutationToException(
message := "Key Management denied access to the terminal KMS Key."
+ " Mutation is halted. Check encrypt access to KMS ARN: " + terminalKmsArn + "."
+ "\n" + errorContext
);
}
}
return Types.KeyStoreAdminException(
Expand Down
Loading