Revert "Remove KMC Key Client from DCS"

[mp-jpl]

Reverts #57 - incorrectly removed key client as part of removing crypto client

In general, the fix is to ensure that every FileInputStream and FileOutputStream created in this method is closed deterministically, even if an exception occurs. In Java 7+, the idiomatic way is to use try-with-resources, which automatically closes AutoCloseable resources at the end of the block.

Concretely for this file:

1. For loading an existing keystore (line 97), first create a FileInputStream variable in a try-with-resources block and pass that variable to keystore.load. This guarantees the stream is closed whether load succeeds or fails:

   try (FileInputStream fis = new FileInputStream(keystoreFile)) {
       keystore.load(fis, keystorePassword.toCharArray());
   }

2. For creating a new keystore file (line 108), wrap the FileOutputStream in a try-with-resources and pass the variable to keystore.store:

   try (FileOutputStream fos = new FileOutputStream(keystoreFile)) {
       keystore.store(fos, keystorePassword.toCharArray());
   }

3. For storing the updated keystore at the end (line 122), also use a try-with-resources FileOutputStream:

   try (FileOutputStream fos = new FileOutputStream(keystoreFilename)) {
       keystore.store(fos, keystorePassword.toCharArray());
   }

These changes do not alter behavior except to ensure files are properly closed. No new imports are needed because FileInputStream and FileOutputStream are already imported. All modifications occur within KmcKeystoreCreate.main.

In general, the correct fix is to ensure that every FileOutputStream created is closed in all execution paths, including when exceptions are thrown. In Java 7+, the idiomatic way is to wrap the creation and use of the stream in a try-with-resources block, which guarantees close() is called.

For this file, we should:

• Replace keystore.store(new FileOutputStream(keystoreFile), ...) in the else branch (around line 108) with a try-with-resources that creates a FileOutputStream variable, passes it to keystore.store, and lets the try-with-resources close it.
• Replace keystore.store(new FileOutputStream(keystoreFilename), ...) in the final try block (around line 122) similarly.
• Keep the existing exception handling and behavior (System.exit and printed messages) unchanged; only add the try-with-resources around the FileOutputStream usage.
• No new imports are required because try-with-resources works with java.io.FileOutputStream, which is already imported.

Concretely:

• In the block creating a new keystore (lines 106–112), wrap the keystore.store(...) call in try (FileOutputStream fos = new FileOutputStream(keystoreFile)) { ... }, and keep the existing catch as-is.
• In the block that stores the updated keystore after adding the key (lines 115–130), do the same around the keystore.store(...) call, keeping the existing catch clauses.

In general, the fix is to ensure that every OutputStream or Writer you create is reliably closed, even if an exception occurs. In Java 7+, the preferred approach is a try‑with‑resources statement, which automatically closes any AutoCloseable declared in the try header.

Specifically for KmcKeystoreCreate.main, we need to wrap the FileOutputStream used on line 122 in a try‑with‑resources block so that it is always closed after keystore.store(...) completes, whether normally or exceptionally. We should not otherwise alter the behavior: the same exceptions are still caught by the existing catch blocks. To do this without changing logic, we will:

• Replace line 122 with a try‑with‑resources block that:
  • Declares a local FileOutputStream variable fos = new FileOutputStream(keystoreFilename);
  • Calls keystore.store(fos, keystorePassword.toCharArray()); inside the block.
• Keep the surrounding try { ... } catch structure intact.
• No new imports are required, since FileOutputStream is already imported.

The similar use of new FileOutputStream(keystoreFile) earlier (line 108) is also technically unclosed, but CodeQL has highlighted only line 122 for this query. Following your instruction to fix the reported error specifically, the change will be limited to the region around line 122.

In general, to fix this class of issue you ensure that at least one operand in the multiplication is a long, so the operation is performed in 64-bit arithmetic and cannot overflow for any values that fit in long. This is done by casting one operand to long before the multiplication, or by using a long literal such as 8L instead of 8.

In this file, there are four multiplications of the form key.getEncoded().length * 8 that are passed into assertEquals calls: at lines 42, 72, 84, and 91. To fix them without changing functionality, we should cast the length to long before multiplying: (long) key.getEncoded().length * 8. This ensures the multiplication is performed in long. The expected values (keyLength or constants like 128/512) are already int and will be widened as needed; we do not need to change them. No additional imports or new methods are required.

To fix the problem, we should remove the always‑true comparison and replace it with an assertion that actually checks something meaningful about the returned list. Common sensible checks in tests like this are that the list is not null, and optionally that its size is greater than zero if the environment is expected to contain at least one key.

The minimal, non‑behavior‑changing improvement is to assert that the list is not null. This preserves the current acceptance of empty lists (size 0) while converting the vacuous assertion into one that can fail if locateAllKeyIds() returns null. Concretely:

• In testLocateAllKeyIds, replace assertTrue(keyIds.size() >= 0); with assertNotNull(keyIds);.
• In testGetAllKeys, replace assertTrue(keys.size() >= 0); with assertNotNull(keys);.

No new imports or helper methods are required because assertNotNull is already statically imported at the top of the file.

In general, to fix this kind of problem you should either remove the always‑true comparison or replace it with an assertion that reflects the real intent of the test (for example, checking that the collection is not empty when that is expected). Keeping an always‑true assertion adds no value and may hide the lack of real verification.

For this specific case in KeyRetrievalTest.testGetAllKeys, the simplest and safest fix without changing existing functionality is to remove the assertion assertTrue(keys.size() >= 0);. The test will still call kmipClient.getAllKeys(), measure and print timing, and iterate over the result as before. We avoid assuming that the list should be non‑empty, because we have no information about the environment or how many keys ought to exist; asserting non‑emptiness could make the test brittle. The only code change needed is to delete line 66 within testGetAllKeys in kmc-key-client/src/test/java/gov/nasa/jpl/ammos/kmc/keyclient/test/KeyRetrievalTest.java. No new methods, imports, or definitions are required.

In general, to fix long instanceof chains when you cannot modify the underlying type hierarchy, you can replace them with a dispatch mechanism like a Map<Class<?>, Handler> that maps runtime types to handler functions. This concentrates all type-specific behavior in a single, extensible structure and avoids having scattered instanceof chains.

For this code, the best targeted fix is to refactor the body of setAttributes so that it does not use a chained if/else if with instanceof. Instead, we can introduce a private static Map<Class<? extends Attribute>, AttributeProcessor> that associates each handled Attribute subclass with a lambda that knows how to extract and assign the corresponding KmcKey field and log it. Then setAttributes simply looks up the processor by a.getClass() and invokes it if present. This keeps behavior identical: same fields set, same logging, and no changes to external interfaces, but significantly improves extensibility and removes the explicit chain.

Concretely:

• Add a private functional interface AttributeProcessor inside KmcKey.
• Add a private static final Map<Class<? extends Attribute>, AttributeProcessor> ATTRIBUTE_PROCESSORS initialized with entries for Name, ObjectType, State, CryptographicUsageMask, InitialDate, LastChangeDate, and ContactInformation. Each lambda will cast the Attribute, read KMIPAttributeValue[] values = a.getValues();, and set the same fields and logs as before.
• Rewrite setAttributes to iterate over attributes, obtain the corresponding processor from the map with ATTRIBUTE_PROCESSORS.get(a.getClass()), and, if non-null, call it; otherwise, ignore unknown attributes (matching current behavior, which simply does nothing for other types).

All changes are local to KmcKey.java within the shown region; no new imports or external dependencies are needed because we use only existing types and standard Java constructs.

In general, the fix is to surround the Integer.parseInt call with a try/catch that catches NumberFormatException and handles invalid numeric data gracefully. Handling can include logging the bad value, keeping the previous value, or assigning a safe default, depending on how the rest of the code uses usageMask.

For this specific code, the best minimal‑impact fix is inside setAttributes: when encountering a CryptographicUsageMask, keep the existing parsing logic but catch NumberFormatException. On failure, log an error (including the bad string value) and avoid changing usageMask so that any earlier/default value remains in effect. This preserves current behavior for valid data, adds robustness for invalid data, and does not change the method signature or other logic. All changes are within KmcKey.java around line 157; no new imports are needed because NumberFormatException is in java.lang.

In general, to fix spurious Javadoc @param tag errors, ensure that each @param name exactly matches a method parameter or type parameter name, and remove or correct any tags that refer to non-existent or misspelled parameters.

For this specific case in KmcKmipKeyClient.java, the method signature is:

public final KmcKey setKeyState(final String keyRef, final String state, final String revokeReason)

but the Javadoc says:

/**
 * Sets the key to the specified state.
 * @param keyRef KeyRef is only used in error messages.  It doesn't affect the operation.
 * @param operation The operation to be performed on the key.
 * @param revokeReason The reason to revoke the key.
 * @throws KmcKeyClientException if the operation failed.
 */

We should change @param operation to @param state so that the Javadoc correctly documents the second parameter. The descriptive text "The operation to be performed on the key." can remain as-is or be slightly adjusted, but it is not necessary for the CodeQL fix; only the parameter name must match. No new imports or additional methods are needed; this is a pure comment change confined to the shown snippet.

[sonarqubecloud]

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud