-
Notifications
You must be signed in to change notification settings - Fork 19
feat: Implement onboarding and enrollment workflow #332
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
base: trunk
Are you sure you want to change the base?
Conversation
| SecretKey key = _aesKeyFromBase64(keyBase64); | ||
| IvParameterSpec iv = ivNonce != null ? _ivFromBase64(ivNonce) : EMPTY_IV; | ||
| Cipher cipher = Cipher.getInstance("AES/SIC/PKCS7Padding", "BC"); | ||
| cipher.init(mode, key, iv); |
Check failure
Code scanning / CodeQL
Using a static initialization vector for encryption High
static initialization vector
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
In general terms, the problem is the use of a static, all-zero IV (EMPTY_IV) whenever no IV is supplied. For AES in SIC (CTR) mode, the IV/nonce must be unique for each encryption with the same key; using a fixed IV breaks confidentiality. The fix is to ensure that an unpredictable, random IV is always used for encryption, and that the same IV is only reused for decryption of the corresponding ciphertext. If the caller doesn’t supply an IV, this method should generate one instead of falling back to a static one.
The single best way to fix this without changing existing functionality more than necessary is:
- Remove the static
EMPTY_IVconstant (or stop using it for encryption). - In
createAesCipher, whenmode == Cipher.ENCRYPT_MODEandivNonceisnull, generate a fresh, random IV of the correct length usingSecureRandom, and construct anIvParameterSpecfrom it. - When
ivNonceis non-null, keep the existing behavior and use_ivFromBase64(ivNonce). - For decryption (
DECRYPT_MODE), keep using the IV provided byivNonce; if it isnull, it’s safer to fail (e.g., throwInvalidAlgorithmParameterException) rather than silently using a static IV. However, to avoid changing external behavior unexpectedly, we can still avoidEMPTY_IVand instead require an IV, or at least avoid the static zero IV. The minimal security-improving change is to eliminateEMPTY_IVand never use a static all-zero vector.
We are constrained to modify only at_client/src/main/java/org/atsign/client/util/EncryptionUtil.java within the shown snippet. Concretely:
- Delete or stop using the line defining
EMPTY_IV. - Update
createAesCipherso line 126 no longer referencesEMPTY_IV, and instead:- If
ivNonce != null, use_ivFromBase64(ivNonce)(as before). - If
ivNonce == null:- If
mode == Cipher.ENCRYPT_MODE, generate a 16-byte random IV usingSecureRandomand wrap it inIvParameterSpec. - Otherwise (e.g., decrypt), throw an
InvalidAlgorithmParameterExceptionindicating an IV is required.
- If
- If
- No new imports are needed because
SecureRandomis already imported, andIvParameterSpecis already imported.
This addresses CodeQL’s complaint by ensuring that no static, deterministic IV is ever used for encryption, while keeping the current method signature and base64-handling intact.
-
Copy modified lines R125-R137
| @@ -14,7 +14,6 @@ | ||
| import java.util.Base64; | ||
|
|
||
| public class EncryptionUtil { | ||
| static private final IvParameterSpec EMPTY_IV = new IvParameterSpec(new byte[16]); | ||
|
|
||
| public static final String SIGNING_ALGO_RSA = "rsa2048"; | ||
| public static final String HASHING_ALGO_SHA256 = "sha256"; | ||
| @@ -123,7 +122,19 @@ | ||
|
|
||
| private static Cipher createAesCipher(int mode, String keyBase64, String ivNonce) throws NoSuchAlgorithmException, NoSuchProviderException, NoSuchPaddingException, InvalidKeyException, InvalidAlgorithmParameterException { | ||
| SecretKey key = _aesKeyFromBase64(keyBase64); | ||
| IvParameterSpec iv = ivNonce != null ? _ivFromBase64(ivNonce) : EMPTY_IV; | ||
| IvParameterSpec iv; | ||
| if (ivNonce != null) { | ||
| iv = _ivFromBase64(ivNonce); | ||
| } else { | ||
| if (mode == Cipher.ENCRYPT_MODE) { | ||
| byte[] ivBytes = new byte[16]; | ||
| SecureRandom secureRandom = new SecureRandom(); | ||
| secureRandom.nextBytes(ivBytes); | ||
| iv = new IvParameterSpec(ivBytes); | ||
| } else { | ||
| throw new InvalidAlgorithmParameterException("IV is required for this operation"); | ||
| } | ||
| } | ||
| Cipher cipher = Cipher.getInstance("AES/SIC/PKCS7Padding", "BC"); | ||
| cipher.init(mode, key, iv); | ||
| return cipher; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akafredperry support for null or 0'd IV is no longer required, this is a legacy from ancient times
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gkc yes understood but not simply a case of removing the capability as Java SDK needs to be enhanced.
I only realised yesterday that it requires changes to support the IV in the meta data and to create / expect an IV for shared and and self keys. I created this #342. But if you think this work should be combined into a single PR I can set this PR to draft and pick this up next week. Ideally I need a bit of your time to confirm some things
28409e1 to
6196c6a
Compare
6196c6a to
0b5857b
Compare
4996153 to
1a9ffc4
Compare
| } | ||
|
|
||
| private static EnrollmentId inferEnrollmentId(String key) { | ||
| Matcher matcher = Pattern.compile("^([^\\.]+).+").matcher(key); |
Check failure
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data High
regular expression
user-provided value
…nd enrollment workflow
3f4ef1a to
47429fa
Compare
- What I did
Added support for PKAMS per app per device, specifically
- How I did it
typed API)
- How to verify it
Run all the tests.
NOTE: I have verified scenarios in which the onboarding and enrollment workflow is done by a combination of the dart activate_cli and the new Java Activate.
- Description for the changelog
closes #324 closes #71