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

Added support for encryption algorithms for symmetric keys #17209

Merged
merged 13 commits into from
Nov 13, 2020
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,10 @@
<!-- InvalidKeyException is not a runtime exception, issue link: https://github.com/Azure/azure-sdk-for-java/issues/5178 -->
<suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLogger"
files="com.azure.security.keyvault.keys.cryptography.AesCbc.java"/>
<suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLogger"
files="com.azure.security.keyvault.keys.cryptography.AesCbcPad.java"/>
<suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLogger"
files="com.azure.security.keyvault.keys.cryptography.AesGcm.java"/>

<!-- MSAL extension temporarily living in our package. Will not take dependency on azure-core once migrated to MSAL repo -->
<suppress checks="com.azure.tools.checkstyle.checks.ThrowFromClientLogger"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2383,7 +2383,7 @@
<Method name="getUserName" />
<Bug pattern="NM_CONFUSING" />
</Match>

<!-- Disabling false positives in azure-core -->
<!-- This Issue has been resolved as per spotbugs's recommended solution but the static checker still flags it, its a known issue with this rule. -->
<Match>
Expand Down Expand Up @@ -2414,4 +2414,13 @@
<Method name="~(get|post)" />
<Bug pattern="RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE" />
</Match>

<!-- Service accepts null byte arrays as input parameters -->
<Match>
<Or>
<Class name="com.azure.security.keyvault.keys.cryptography.DecryptOptions" />
<Class name="com.azure.security.keyvault.keys.cryptography.EncryptOptions" />
</Or>
<Bug pattern="PZLA_PREFER_ZERO_LENGTH_ARRAYS" />
</Match>
</FindBugsFilter>
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package com.azure.security.keyvault.keys.cryptography;

class Aes128CbcPad extends AesCbcPad {
private static final int KEY_SIZE = 128;
public static final String ALGORITHM_NAME = "A128CBCPAD";

Aes128CbcPad() {
super(ALGORITHM_NAME, KEY_SIZE);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package com.azure.security.keyvault.keys.cryptography;

class Aes128Gcm extends AesGcm {
private static final int KEY_SIZE = 128;
public static final String ALGORITHM_NAME = "A128GCM";

Aes128Gcm() {
super(ALGORITHM_NAME, KEY_SIZE);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@
import java.security.Provider;
import java.util.Arrays;

class AesKw128 extends AesKw {
class Aes128Kw extends AesKw {

public static final String ALGORITHM_NAME = "A128KW";

static final int KEY_SIZE_IN_BYTES = 128 >> 3;

AesKw128() {
Aes128Kw() {
super(ALGORITHM_NAME);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package com.azure.security.keyvault.keys.cryptography;

class Aes192CbcPad extends AesCbcPad {
private static final int KEY_SIZE = 192;
public static final String ALGORITHM_NAME = "A192CBCPAD";

Aes192CbcPad() {
super(ALGORITHM_NAME, KEY_SIZE);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package com.azure.security.keyvault.keys.cryptography;

class Aes192Gcm extends AesGcm {
private static final int KEY_SIZE = 192;
public static final String ALGORITHM_NAME = "A192GCM";

Aes192Gcm() {
super(ALGORITHM_NAME, KEY_SIZE);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@
import java.security.Provider;
import java.util.Arrays;

class AesKw192 extends AesKw {
class Aes192Kw extends AesKw {

public static final String ALGORITHM_NAME = "A192KW";

static final int KEY_SIZE_IN_BYTES = 192 >> 3;

AesKw192() {
Aes192Kw() {
super(ALGORITHM_NAME);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package com.azure.security.keyvault.keys.cryptography;

class Aes256CbcPad extends AesCbcPad {
private static final int KEY_SIZE = 256;
public static final String ALGORITHM_NAME = "A256CBCPAD";

Aes256CbcPad() {
super(ALGORITHM_NAME, KEY_SIZE);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package com.azure.security.keyvault.keys.cryptography;

class Aes256Gcm extends AesGcm {
private static final int KEY_SIZE = 256;
public static final String ALGORITHM_NAME = "A256GCM";

Aes256Gcm() {
super(ALGORITHM_NAME, KEY_SIZE);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@
import java.security.Provider;
import java.util.Arrays;

class AesKw256 extends AesKw {
class Aes256Kw extends AesKw {

public static final String ALGORITHM_NAME = "A256KW";

static final int KEY_SIZE_IN_BYTES = 256 >> 3;

AesKw256() {
Aes256Kw() {
super(ALGORITHM_NAME);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,25 +16,30 @@
import java.util.Arrays;

abstract class AesCbc extends SymmetricEncryptionAlgorithm {

final int keySizeInBytes;
final int keySize;
static class AesCbcDecryptor implements ICryptoTransform {

protected AesCbc(String name, int size) {
super(name);

keySize = size;
keySizeInBytes = size >> 3;
}

static class AesCbcEncryptor implements ICryptoTransform {
private final Cipher cipher;

AesCbcDecryptor(byte[] key, byte[] iv, Provider provider)
throws NoSuchAlgorithmException, NoSuchPaddingException, InvalidKeyException,
InvalidAlgorithmParameterException {
AesCbcEncryptor(byte[] key, byte[] iv, Provider provider) throws NoSuchAlgorithmException,
NoSuchPaddingException, InvalidKeyException, InvalidAlgorithmParameterException {

// Create the cipher using the Provider if specified
if (provider == null) {
cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
cipher = Cipher.getInstance("AES/CBC/NoPadding");
Copy link
Member

Choose a reason for hiding this comment

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

@lusitanian should this be zero-padding? When we spoke about .NET, you said zero-padding was what the service was using.

Copy link
Member

Choose a reason for hiding this comment

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

...if this is right, I'll have to change .NET's implementation to NoPadding as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Going from the name only it made sense to me that we should not use padding for AES-CBC and use padding for AES-CBC-PAD. Is that not the case?

Copy link
Member

Choose a reason for hiding this comment

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

When I asked MHSM about it, the reply was that zero-padding seems to be closer. I'm honestly not sure. If you're writing tests, maybe try it against the service and see what it does with CBC vs CBCPAD.

} else {
cipher = Cipher.getInstance("AES/CBC/PKCS5Padding", provider);
cipher = Cipher.getInstance("AES/CBC/NoPadding", provider);
}

cipher.init(Cipher.DECRYPT_MODE, new SecretKeySpec(key, "AES"), new IvParameterSpec(iv));
cipher.init(Cipher.ENCRYPT_MODE, new SecretKeySpec(key, "AES"), new IvParameterSpec(iv));
}

@Override
Expand All @@ -43,22 +48,20 @@ public byte[] doFinal(byte[] plaintext) throws IllegalBlockSizeException, BadPad
}
}

static class AesCbcEncryptor implements ICryptoTransform {

static class AesCbcDecryptor implements ICryptoTransform {
private final Cipher cipher;

AesCbcEncryptor(byte[] key, byte[] iv, Provider provider)
throws NoSuchAlgorithmException, NoSuchPaddingException, InvalidKeyException,
InvalidAlgorithmParameterException {
AesCbcDecryptor(byte[] key, byte[] iv, Provider provider) throws NoSuchAlgorithmException,
NoSuchPaddingException, InvalidKeyException, InvalidAlgorithmParameterException {

// Create the cipher using the Provider if specified
if (provider == null) {
cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
cipher = Cipher.getInstance("AES/CBC/NoPadding");
} else {
cipher = Cipher.getInstance("AES/CBC/PKCS5Padding", provider);
cipher = Cipher.getInstance("AES/CBC/NoPadding", provider);
}

cipher.init(Cipher.ENCRYPT_MODE, new SecretKeySpec(key, "AES"), new IvParameterSpec(iv));
cipher.init(Cipher.DECRYPT_MODE, new SecretKeySpec(key, "AES"), new IvParameterSpec(iv));
}

@Override
Expand All @@ -67,22 +70,12 @@ public byte[] doFinal(byte[] plaintext) throws IllegalBlockSizeException, BadPad
}
}

protected AesCbc(String name, int size) {
super(name);
keySize = size;
keySizeInBytes = size >> 3;
}

@Override
public ICryptoTransform createEncryptor(byte[] key, byte[] iv, byte[] authenticationData)
throws InvalidKeyException, NoSuchAlgorithmException, NoSuchPaddingException,
InvalidAlgorithmParameterException {

if (key == null || key.length < keySizeInBytes) {
throw new InvalidKeyException("key must be at least " + keySize + " bits in length");
}

return new AesCbcEncryptor(Arrays.copyOfRange(key, 0, keySizeInBytes), iv, null);
return createEncryptor(key, iv, authenticationData, null);
vcolin7 marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
Expand All @@ -91,7 +84,7 @@ public ICryptoTransform createEncryptor(byte[] key, byte[] iv, byte[] authentica
InvalidAlgorithmParameterException {

if (key == null || key.length < keySizeInBytes) {
throw new InvalidKeyException("key must be at least " + keySize + " bits in length");
throw new InvalidKeyException("Key must be at least " + keySize + " bits in length.");
}

return new AesCbcEncryptor(Arrays.copyOfRange(key, 0, keySizeInBytes), iv, provider);
Expand All @@ -102,11 +95,7 @@ public ICryptoTransform createDecryptor(byte[] key, byte[] iv, byte[] authentica
throws InvalidKeyException, NoSuchAlgorithmException, NoSuchPaddingException,
InvalidAlgorithmParameterException {

if (key == null || key.length < keySizeInBytes) {
throw new InvalidKeyException("key must be at least " + keySize + " bits in length");
}

return new AesCbcDecryptor(Arrays.copyOfRange(key, 0, keySizeInBytes), iv, null);
return createDecryptor(key, iv, authenticationData, authenticationTag, null);
}

@Override
Expand All @@ -116,7 +105,7 @@ public ICryptoTransform createDecryptor(byte[] key, byte[] iv, byte[] authentica
InvalidAlgorithmParameterException {

if (key == null || key.length < keySizeInBytes) {
throw new InvalidKeyException("key must be at least " + keySize + " bits in length");
throw new InvalidKeyException("Key must be at least " + keySize + " bits in length.");
}

return new AesCbcDecryptor(Arrays.copyOfRange(key, 0, keySizeInBytes), iv, provider);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package com.azure.security.keyvault.keys.cryptography;

/**
* A class containing configuration parameters that can be applied when decrypting AES-CBC keys with and without
* padding.
*/
public class AesCbcDecryptOptions extends DecryptOptions {
/**
* Creates an instance of {@link AesCbcDecryptOptions} with the given parameters.
*
* @param iv Initialization vector for the decryption operation.
*/
public AesCbcDecryptOptions(byte[] iv) {
Copy link
Member

Choose a reason for hiding this comment

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

Do you think these will be discoverable? In .NET, we waffled on classes like this vs. factories and opted for the latter for discoverability. Would htat work better here. You could, for example, mix that and builders by having a factory returning the right class that you can then set options, e.t.:

EncryptOptions
  .createA128GcmOptions(iv, key)
  .setAdditionalAuthenticationData(data);

I'm also wondering how they specify the key size with this.

Copy link
Member Author

@vcolin7 vcolin7 Nov 11, 2020

Choose a reason for hiding this comment

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

The Key size is specified in the clients. For example:

CryptographyClient.encrypt(EncryptionAlgorithm algorithm, String plaintext, EncryptOptions options)

Copy link
Member Author

@vcolin7 vcolin7 Nov 11, 2020

Choose a reason for hiding this comment

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

Do you have any thoughts on this @srnagar, @JonathanGiles?

Copy link
Member

Choose a reason for hiding this comment

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

Since these are inputs to the API, how does the user know what are the available subtypes of EncryptOptions/DecryptOptions. In other places, we have used a type flag to switch between strongly typed sub-classes.
See example here:
https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/formrecognizer/azure-ai-formrecognizer/src/main/java/com/azure/ai/formrecognizer/models/FieldValue.java

super(iv, null, null);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package com.azure.security.keyvault.keys.cryptography;

/**
* A class containing configuration parameters that can be applied when encrypting AES-CBC keys with and without
* padding.
*/
public class AesCbcEncryptOptions extends EncryptOptions {
/**
* Creates an instance of {@link AesCbcEncryptOptions} with the given parameters.
*
* @param iv Initialization vector for the encryption operation.
*/
public AesCbcEncryptOptions(byte[] iv) {
super(iv, null);
}
}
Loading