diff --git a/AGENTS.md b/AGENTS.md index ba8e193..2dd3b66 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -26,7 +26,7 @@ Unit tests must not depend on a real BMC. Exercising the RMCP+ session code agai ## Code quality reports -Code quality reports (checkstyle, pmd/cpd, spotbugs) are generated by `mvn verify site` into ./target/checkstyle-result.xml, ./target/pmd.xml, ./target/cpd.xml and ./target/spotbugsXml.xml. Checkstyle and PMD are gated (the build fails on any error); CPD is gated on duplications of 100 tokens or more (`cpd-check` at `verify`), while the site report lists those of 50 tokens or more; SpotBugs is not yet gated (issue #116 tracks the clean-up). Extract shared code instead of copying it: `AbstractSensorRecord` holds what the Full, Compact and Event-Only sensor records share, and `IpmiCommandCoder.validateResponse()` the response checks of every command. Do not add new violations: check the reports for the files you changed before committing and submitting your code. The SpotBugs site report runs spotbugs-maven-plugin 4.10.4.1 (pinned in ``, as the 4.9.3.0 of the parent POM cannot read the class files of JDK 21+); to run SpotBugs alone, use `mvn com.github.spotbugs:spotbugs-maven-plugin:4.10.4.1:spotbugs`. +Code quality reports (checkstyle, pmd/cpd, spotbugs) are generated by `mvn verify site` into ./target/checkstyle-result.xml, ./target/pmd.xml, ./target/cpd.xml and ./target/spotbugsXml.xml. Checkstyle and PMD are gated (the build fails on any error); CPD is gated on duplications of 100 tokens or more (`cpd-check` at `verify`), while the site report lists those of 50 tokens or more; SpotBugs is gated on any bug at the default threshold (`spotbugs:check` at `verify`). Extract shared code instead of copying it: `AbstractSensorRecord` holds what the Full, Compact and Event-Only sensor records share, and `IpmiCommandCoder.validateResponse()` the response checks of every command. Do not add new violations: check the reports for the files you changed before committing and submitting your code. The SpotBugs gate and site report run spotbugs-maven-plugin 4.10.4.1 (pinned in `` and ``, as the 4.9.3.0 of the parent POM cannot read the class files of JDK 21+); to run SpotBugs alone, use `mvn com.github.spotbugs:spotbugs-maven-plugin:4.10.4.1:check`. Fix SpotBugs findings; when one is intentional, suppress it with `@SuppressFBWarnings` (`spotbugs-annotations`, provided scope) and a `justification`. `CT_CONSTRUCTOR_THROW` and `EI_EXPOSE_REP` (which also matches `EI_EXPOSE_REP2`) are suppressed for the whole protocol core in `org/metricshub/ipmi/core/package-info.java`; SpotBugs reports a suppression that matches nothing, so remove it with the code it covered. ## Documentation diff --git a/README.md b/README.md index 303cbe3..6b86d0d 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ The BMC must have IPMI over LAN enabled and an account with the User privilege: ## Upgrading -Version 1.2.03 makes the `protected` fields of the protocol classes (`AbstractIpmiRunner`, `MessageHandler`, `IpmiLanMessage`, `ConfidentialityAlgorithm`, `IntegrityAlgorithm`) `private`. Subclasses must use the new `protected` accessors instead; see [Upgrading from 1.2.02](https://metricshub.org/ipmi-java/upgrading.html#upgrading-from-1-2-02) for the list. The `IpmiClient` API is unchanged. The Full, Compact and Event-Only sensor records now share the `AbstractSensorRecord` superclass, and commands can check responses with `IpmiCommandCoder.validateResponse()`; both are described on the same page. +Version 1.2.03 makes the `protected` fields of the protocol classes (`AbstractIpmiRunner`, `MessageHandler`, `IpmiLanMessage`, `ConfidentialityAlgorithm`, `IntegrityAlgorithm`) `private`. Subclasses must use the new `protected` accessors instead; see [Upgrading from 1.2.02](https://metricshub.org/ipmi-java/upgrading.html#upgrading-from-1-2-02) for the list. The `IpmiClient` API is unchanged. The Full, Compact and Event-Only sensor records now share the `AbstractSensorRecord` superclass, and commands can check responses with `IpmiCommandCoder.validateResponse()`; both are described on the same page. The user name and password are now encoded in UTF-8 whatever the platform charset, and the BMC key is used as raw bytes; as a result, `AuthenticationAlgorithm.getKeyExchangeAuthenticationCode()` and `checkKeyExchangeAuthenticationCode()` take the key and password as `byte[]` instead of `String`. `UdpMessenger.getSentPackets()` is removed, and the `CONST1`/`CONST2` constants of `IntegrityAlgorithm` and `ConfidentialityAesCbc128` are now `private`. ## Build instructions @@ -42,6 +42,8 @@ mvn formatter:format The build also fails on [Checkstyle](checkstyle.xml) violations. A justified violation can be suppressed with `// CHECKSTYLE.OFF: ` and `// CHECKSTYLE.ON: ` comments. +The build fails on any SpotBugs bug as well. An intentional one is suppressed with `@SuppressFBWarnings` and a `justification`. + To ignore the whole-tree reformat commit in `git blame`, run once: ```bash diff --git a/pom.xml b/pom.xml index 43bef44..089d002 100644 --- a/pom.xml +++ b/pom.xml @@ -92,6 +92,13 @@ + + + com.github.spotbugs + spotbugs-annotations + 4.10.4 + provided + org.slf4j slf4j-api @@ -177,6 +184,22 @@ + + + com.github.spotbugs + spotbugs-maven-plugin + 4.10.4.1 + + + verify + + check + + + + + + com.github.spotbugs spotbugs-maven-plugin diff --git a/src/main/java/org/metricshub/ipmi/client/IpmiClientConfiguration.java b/src/main/java/org/metricshub/ipmi/client/IpmiClientConfiguration.java index 57241da..232d114 100644 --- a/src/main/java/org/metricshub/ipmi/client/IpmiClientConfiguration.java +++ b/src/main/java/org/metricshub/ipmi/client/IpmiClientConfiguration.java @@ -24,10 +24,15 @@ import org.metricshub.ipmi.core.common.Constants; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + /** * IPMI configuration including the required credentials that need to be used to establish the * communication with the IPMI interface. */ +// EI_EXPOSE_REP also matches EI_EXPOSE_REP2 +@SuppressFBWarnings(value = "EI_EXPOSE_REP", justification = "The password char[] and BMC key byte[] are " + + "deliberately shared, not copied, so that the caller can wipe the only copy of the credentials") public class IpmiClientConfiguration { private String hostname; diff --git a/src/main/java/org/metricshub/ipmi/client/model/Fru.java b/src/main/java/org/metricshub/ipmi/client/model/Fru.java index f542382..cf204fc 100644 --- a/src/main/java/org/metricshub/ipmi/client/model/Fru.java +++ b/src/main/java/org/metricshub/ipmi/client/model/Fru.java @@ -24,6 +24,8 @@ import java.util.List; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + import org.metricshub.ipmi.core.coding.commands.fru.record.BoardInfo; import org.metricshub.ipmi.core.coding.commands.fru.record.ChassisInfo; import org.metricshub.ipmi.core.coding.commands.fru.record.FruRecord; @@ -37,6 +39,9 @@ *
  • The FRU records containing {@link BoardInfo}, {@link ChassisInfo} and/or {@link ProductInfo}.
    • * */ +// EI_EXPOSE_REP also matches EI_EXPOSE_REP2 +@SuppressFBWarnings(value = "EI_EXPOSE_REP", justification = "Result holder: hands out the decoded records, " + + "which are mutable holders themselves, by reference") public class Fru { private FruDeviceLocatorRecord fruLocator; diff --git a/src/main/java/org/metricshub/ipmi/client/model/Sensor.java b/src/main/java/org/metricshub/ipmi/client/model/Sensor.java index 8ee79c4..83bdfb5 100644 --- a/src/main/java/org/metricshub/ipmi/client/model/Sensor.java +++ b/src/main/java/org/metricshub/ipmi/client/model/Sensor.java @@ -22,6 +22,8 @@ * ╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱ */ +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + import org.metricshub.ipmi.core.coding.commands.sdr.GetSensorReadingResponseData; import org.metricshub.ipmi.core.coding.commands.sdr.record.AbstractSensorRecord; import org.metricshub.ipmi.core.coding.commands.sdr.record.CompactSensorRecord; @@ -42,6 +44,9 @@ * $sensorName=$state|$sensorName=$state|...|$sensorName=$state * */ +// EI_EXPOSE_REP also matches EI_EXPOSE_REP2 +@SuppressFBWarnings(value = "EI_EXPOSE_REP", justification = "Result holder: hands out the decoded record and " + + "reading, which are mutable holders themselves, by reference") public class Sensor { private SensorRecord sensorRecord; diff --git a/src/main/java/org/metricshub/ipmi/core/api/sol/SerialOverLan.java b/src/main/java/org/metricshub/ipmi/core/api/sol/SerialOverLan.java index 5344be6..337d2ba 100644 --- a/src/main/java/org/metricshub/ipmi/core/api/sol/SerialOverLan.java +++ b/src/main/java/org/metricshub/ipmi/core/api/sol/SerialOverLan.java @@ -413,7 +413,7 @@ public boolean writeIntArray(int[] buffer) { * @return true if whole string was successfully sent and acknowledged by remote server, false otherwise. */ public boolean writeString(String string) { - return writeBytes(string.getBytes()); + return writeString(string, Charset.defaultCharset()); } /** @@ -541,7 +541,7 @@ public String readString() { * @return all bytes that could be read as {@link String}, but no more than given byteCount. */ public String readString(int byteCount) { - return new String(readBytes(byteCount)); + return readString(Charset.defaultCharset(), byteCount); } /** diff --git a/src/main/java/org/metricshub/ipmi/core/api/sync/MessageListener.java b/src/main/java/org/metricshub/ipmi/core/api/sync/MessageListener.java index 7e29ef9..6e1bb31 100644 --- a/src/main/java/org/metricshub/ipmi/core/api/sync/MessageListener.java +++ b/src/main/java/org/metricshub/ipmi/core/api/sync/MessageListener.java @@ -46,7 +46,7 @@ public class MessageListener implements IpmiResponseListener { private int tag; - private IpmiResponse response; + private volatile IpmiResponse response; /** * Messages that have proper connection handle but arrived before tag was @@ -86,17 +86,21 @@ public ResponseData waitForAnswer(int messageTag) throws Exception { if (messageTag < 0 || messageTag > 63) { throw new IllegalArgumentException("Corrupted message tag"); } - this.tag = messageTag; - for (IpmiResponse quickResponse : quickMessages) { - this.notify(quickResponse); + synchronized (this) { + this.tag = messageTag; + for (IpmiResponse quickResponse : quickMessages) { + this.notify(quickResponse); + } } while (response == null) { Thread.sleep(1); } if (response instanceof IpmiResponseData) { - this.tag = -1; - quickMessages.clear(); + synchronized (this) { + this.tag = -1; + quickMessages.clear(); + } return ((IpmiResponseData) response).getResponseData(); } else /* response instanceof IpmiError */ { throw ((IpmiError) response).getException(); diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoder.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoder.java index ac7f5ac..c70498f 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoder.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoder.java @@ -24,7 +24,6 @@ import org.metricshub.ipmi.core.coding.PayloadCoder; import org.metricshub.ipmi.core.coding.payload.CompletionCode; -import org.metricshub.ipmi.core.coding.payload.IpmiPayload; import org.metricshub.ipmi.core.coding.payload.PlainMessage; import org.metricshub.ipmi.core.coding.payload.lan.IPMIException; import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanResponse; @@ -66,15 +65,10 @@ public PayloadType getSupportedPayloadType() { * class, false otherwise. */ public boolean isCommandResponse(IpmiMessage message) { - if (message.getPayload() instanceof IpmiPayload) { - if (message.getPayload() instanceof IpmiLanResponse) { - return ((IpmiLanResponse) message.getPayload()).getCommand() == getCommandCode(); - } else { - return message.getPayload() instanceof PlainMessage; - } - } else { - return false; + if (message.getPayload() instanceof IpmiLanResponse) { + return ((IpmiLanResponse) message.getPayload()).getCommand() == getCommandCode(); } + return message.getPayload() instanceof PlainMessage; } /** diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ChassisInfo.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ChassisInfo.java index 97017c3..e062ecf 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ChassisInfo.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ChassisInfo.java @@ -105,9 +105,6 @@ public ChassisInfo(final byte[] fruData, final int offset) { true)); break; default: - if (partDataLength == 0) { - continue; - } customInfo .add( decodeString( diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ManagementAccessInfo.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ManagementAccessInfo.java index a1620b1..0355314 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ManagementAccessInfo.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/fru/record/ManagementAccessInfo.java @@ -24,6 +24,8 @@ import org.metricshub.ipmi.core.common.TypeConverter; +import java.nio.charset.StandardCharsets; + /** * Management Access Information record from FRU Multi Record Area */ @@ -56,7 +58,7 @@ public ManagementAccessInfo(byte[] fruData, int offset, int length) { System.arraycopy(fruData, offset + 1, buffer, 0, length - 1); - accessInfo = new String(buffer); + accessInfo = new String(buffer, StandardCharsets.ISO_8859_1); } public ManagementAccessRecordType getRecordType() { diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/session/Rakp1.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/session/Rakp1.java index 7726c1d..0edeaa0 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/session/Rakp1.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/session/Rakp1.java @@ -40,6 +40,7 @@ import org.metricshub.ipmi.core.common.Randomizer; import org.metricshub.ipmi.core.common.TypeConverter; +import java.nio.charset.StandardCharsets; import java.security.InvalidKeyException; import java.security.NoSuchAlgorithmException; @@ -108,9 +109,9 @@ public PrivilegeLevel getRequestedMaximumPrivilegeLevel() { } public void setUsername(String username) { - if (username.length() > 16) { + if (username.getBytes(StandardCharsets.UTF_8).length > 16) { throw new IllegalArgumentException( - "Username is too long. It's length cannot exceed 16"); + "Username is too long. It's length cannot exceed 16 bytes"); } this.username = username; } @@ -119,6 +120,13 @@ public String getUsername() { return username; } + /** + * @return the user name as sent to the BMC, encoded in UTF-8 + */ + byte[] getUsernameBytes() { + return username.getBytes(StandardCharsets.UTF_8); + } + private void setPassword(String password) { this.password = password; } @@ -127,6 +135,13 @@ public String getPassword() { return password; } + /** + * @return the password as the key of the authentication algorithm, encoded in UTF-8 (empty when null) + */ + byte[] getPasswordBytes() { + return password == null ? new byte[0] : password.getBytes(StandardCharsets.UTF_8); + } + private void setConsoleRandomNumber(byte[] randomNumber) { this.consoleRandomNumber = randomNumber; } @@ -216,13 +231,8 @@ public IpmiMessage encodePayload(int messageSequenceNumber, int sessionSequenceN @Override protected IpmiPayload preparePayload(int sequenceNumber) { - byte[] payload = null; - - if (getUsername() == null) { - setUsername(""); - } - - payload = new byte[28 + getUsername().length()]; + byte[] usernameBytes = getUsernameBytes(); + byte[] payload = new byte[28 + usernameBytes.length]; // message tag payload[0] = TypeConverter.intToByte(sequenceNumber); @@ -247,18 +257,8 @@ protected IpmiPayload preparePayload(int sequenceNumber) { payload[25] = 0; // reserved payload[26] = 0; // reserved - payload[27] = TypeConverter.intToByte(getUsername().length()); // username - // length - - if (getUsername().length() > 0) { - System - .arraycopy( - getUsername().getBytes(), - 0, - payload, - 28, - getUsername().length()); // username - } + payload[27] = TypeConverter.intToByte(usernameBytes.length); // username length + System.arraycopy(usernameBytes, 0, payload, 28, usernameBytes.length); // username return new PlainMessage(payload); } @@ -349,7 +349,7 @@ public ResponseData getResponseData(IpmiMessage message) .checkKeyExchangeAuthenticationCode( prepareKeyExchangeAuthenticationCodeBase(data), key, - getPassword())) { + getPasswordBytes())) { throw new IllegalArgumentException("Authentication check failed"); } @@ -362,11 +362,8 @@ public ResponseData getResponseData(IpmiMessage message) */ private byte[] prepareKeyExchangeAuthenticationCodeBase( Rakp1ResponseData responseData) { - int length = 58; - if (getUsername() != null) { - length += getUsername().length(); - } - byte[] keac = new byte[length]; + byte[] usernameBytes = getUsernameBytes(); + byte[] keac = new byte[58 + usernameBytes.length]; byte[] rSID = TypeConverter .intToLittleEndianByteArray( @@ -395,20 +392,8 @@ private byte[] prepareKeyExchangeAuthenticationCodeBase( keac[56] = TypeConverter .intToByte(encodePrivilegeLevel(requestedMaximumPrivilegeLevel) | 0x10); - if (getUsername() != null) { - keac[57] = TypeConverter.intToByte(getUsername().length()); - if (getUsername().length() > 0) { - System - .arraycopy( - getUsername().getBytes(), - 0, - keac, - 58, - getUsername().length()); - } - } else { - keac[57] = 0; - } + keac[57] = TypeConverter.intToByte(usernameBytes.length); + System.arraycopy(usernameBytes, 0, keac, 58, usernameBytes.length); return keac; } @@ -430,7 +415,7 @@ public byte[] calculateSik(Rakp1ResponseData responseData) NoSuchAlgorithmException { byte[] key = null; if (getBmcKey() == null || getBmcKey().length <= 0) { - key = getPassword().getBytes(); + key = getPasswordBytes(); } else { key = getBmcKey(); } @@ -439,7 +424,7 @@ public byte[] calculateSik(Rakp1ResponseData responseData) .getAuthenticationAlgorithm() .getKeyExchangeAuthenticationCode( prepareSikBase(responseData), - new String(key)); + key); } /** @@ -447,12 +432,8 @@ public byte[] calculateSik(Rakp1ResponseData responseData) * Integrity Key */ private byte[] prepareSikBase(Rakp1ResponseData responseData) { - int length = 34; - if (getUsername() != null) { - length += getUsername().length(); - } - - byte[] sikBase = new byte[length]; + byte[] usernameBytes = getUsernameBytes(); + byte[] sikBase = new byte[34 + usernameBytes.length]; System.arraycopy(getConsoleRandomNumber(), 0, sikBase, 0, 16); @@ -467,20 +448,8 @@ private byte[] prepareSikBase(Rakp1ResponseData responseData) { sikBase[32] = TypeConverter .intToByte(encodePrivilegeLevel(requestedMaximumPrivilegeLevel) | 0x10); - if (getUsername() != null) { - sikBase[33] = TypeConverter.intToByte(getUsername().length()); - if (getUsername().length() > 0) { - System - .arraycopy( - getUsername().getBytes(), - 0, - sikBase, - 34, - getUsername().length()); - } - } else { - sikBase[33] = 0; - } + sikBase[33] = TypeConverter.intToByte(usernameBytes.length); + System.arraycopy(usernameBytes, 0, sikBase, 34, usernameBytes.length); return sikBase; } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/commands/session/Rakp3.java b/src/main/java/org/metricshub/ipmi/core/coding/commands/session/Rakp3.java index 36a6c75..7f63161 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/commands/session/Rakp3.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/commands/session/Rakp3.java @@ -210,7 +210,7 @@ protected IpmiPayload preparePayload(int sequenceNumber) prepareKeyExchangeAuthenticationCodeBase( rakp1, rakp1ResponseData), - rakp1.getPassword()); + rakp1.getPasswordBytes()); byte[] result = null; @@ -239,11 +239,8 @@ protected IpmiPayload preparePayload(int sequenceNumber) private byte[] prepareKeyExchangeAuthenticationCodeBase( Rakp1 rakp1Message, Rakp1ResponseData responseData) { - int length = 22; - if (rakp1Message.getUsername() != null) { - length += rakp1Message.getUsername().length(); - } - byte[] keac = new byte[length]; + byte[] username = rakp1Message.getUsernameBytes(); + byte[] keac = new byte[22 + username.length]; System .arraycopy( @@ -271,20 +268,8 @@ private byte[] prepareKeyExchangeAuthenticationCodeBase( .getRequestedMaximumPrivilegeLevel()) | 0x10); - if (rakp1Message.getUsername() != null) { - keac[21] = TypeConverter.intToByte(rakp1Message.getUsername().length()); - if (rakp1Message.getUsername().length() > 0) { - System - .arraycopy( - rakp1Message.getUsername().getBytes(), - 0, - keac, - 22, - rakp1Message.getUsername().length()); - } - } else { - keac[21] = 0; - } + keac[21] = TypeConverter.intToByte(username.length); + System.arraycopy(username, 0, keac, 22, username.length); return keac; } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/protocol/decoder/ProtocolDecoder.java b/src/main/java/org/metricshub/ipmi/core/coding/protocol/decoder/ProtocolDecoder.java index 4aff7fe..c79df37 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/protocol/decoder/ProtocolDecoder.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/protocol/decoder/ProtocolDecoder.java @@ -192,6 +192,8 @@ protected static int decodeSessionID(byte[] rawMessage, int offset) { * - {@link ConfidentialityAlgorithm} required to decrypt * payload. * @return Payload decoded into {@link IpmiLanResponse}. + * @throws IllegalArgumentException + * when the payload is empty */ protected IpmiPayload decodePayload( byte[] rawData, @@ -199,14 +201,14 @@ protected IpmiPayload decodePayload( int length, ConfidentialityAlgorithm confidentialityAlgorithm, PayloadType payloadType) { - byte[] payload = null; - if (length > 0) { - payload = new byte[length]; + if (length <= 0) { + throw new IllegalArgumentException("Empty payload"); + } + byte[] payload = new byte[length]; - System.arraycopy(rawData, offset, payload, 0, length); + System.arraycopy(rawData, offset, payload, 0, length); - payload = confidentialityAlgorithm.decrypt(payload); - } + payload = confidentialityAlgorithm.decrypt(payload); if (payloadType == PayloadType.Sol) { return new SolInboundMessage(payload); diff --git a/src/main/java/org/metricshub/ipmi/core/coding/security/AuthenticationAlgorithm.java b/src/main/java/org/metricshub/ipmi/core/coding/security/AuthenticationAlgorithm.java index 81a0ad6..3ceb68d 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/security/AuthenticationAlgorithm.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/security/AuthenticationAlgorithm.java @@ -76,12 +76,12 @@ private AuthenticationAlgorithm(Mac mac) { * @param data - The base for authentication algorithm. Depends on RAKP * Message. * @param key - the Key Exchange Authentication Code to check. - * @param password - password of the user establishing a session + * @param password - password of the user establishing a session, as bytes * @return True if authentication check was successful, false otherwise. * @throws NoSuchAlgorithmException when initiation of the algorithm fails * @throws InvalidKeyException when creating of the algorithm key fails */ - public boolean checkKeyExchangeAuthenticationCode(byte[] data, byte[] key, String password) + public boolean checkKeyExchangeAuthenticationCode(byte[] data, byte[] key, byte[] password) throws NoSuchAlgorithmException, InvalidKeyException { byte[] check = getKeyExchangeAuthenticationCode(data, password); @@ -93,16 +93,15 @@ public boolean checkKeyExchangeAuthenticationCode(byte[] data, byte[] key, Strin * * @param data - The base for authentication algorithm. Depends on RAKP * Message. - * @param password - password of the user establishing a session + * @param key - the password of the user establishing a session, or the BMC key (Kg), as bytes + * @return the Key Exchange Authentication Code * @throws NoSuchAlgorithmException when initiation of the algorithm fails * @throws InvalidKeyException when creating of the algorithm key fails */ - public byte[] getKeyExchangeAuthenticationCode(byte[] data, String password) + public byte[] getKeyExchangeAuthenticationCode(byte[] data, byte[] key) throws NoSuchAlgorithmException, InvalidKeyException { - final byte[] key = password.getBytes(); - SecretKeySpec sKey = new SecretKeySpec(key, getAlgorithmName()); mac.init(sKey); diff --git a/src/main/java/org/metricshub/ipmi/core/coding/security/AuthenticationRakpNone.java b/src/main/java/org/metricshub/ipmi/core/coding/security/AuthenticationRakpNone.java index cfd330d..7b7d2ca 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/security/AuthenticationRakpNone.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/security/AuthenticationRakpNone.java @@ -44,7 +44,7 @@ public byte getCode() { * the RAKP-None algorithm. */ @Override - public boolean checkKeyExchangeAuthenticationCode(byte[] data, byte[] key, String password) { + public boolean checkKeyExchangeAuthenticationCode(byte[] data, byte[] key, byte[] password) { return true; } @@ -53,7 +53,7 @@ public boolean checkKeyExchangeAuthenticationCode(byte[] data, byte[] key, Strin * using the RAKP-None algorithm. */ @Override - public byte[] getKeyExchangeAuthenticationCode(byte[] data, String password) { + public byte[] getKeyExchangeAuthenticationCode(byte[] data, byte[] key) { return new byte[0]; } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/security/ConfidentialityAesCbc128.java b/src/main/java/org/metricshub/ipmi/core/coding/security/ConfidentialityAesCbc128.java index 11798a3..57cbf77 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/security/ConfidentialityAesCbc128.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/security/ConfidentialityAesCbc128.java @@ -39,7 +39,7 @@ */ public class ConfidentialityAesCbc128 extends ConfidentialityAlgorithm { - protected static final byte[] CONST2 = new byte[20]; + private static final byte[] CONST2 = new byte[20]; static { Arrays.fill(CONST2, (byte) 2); } diff --git a/src/main/java/org/metricshub/ipmi/core/coding/security/IntegrityAlgorithm.java b/src/main/java/org/metricshub/ipmi/core/coding/security/IntegrityAlgorithm.java index 949e15b..b739adf 100644 --- a/src/main/java/org/metricshub/ipmi/core/coding/security/IntegrityAlgorithm.java +++ b/src/main/java/org/metricshub/ipmi/core/coding/security/IntegrityAlgorithm.java @@ -37,7 +37,7 @@ */ public abstract class IntegrityAlgorithm { - protected static final byte[] CONST1 = new byte[20]; + private static final byte[] CONST1 = new byte[20]; static { Arrays.fill(CONST1, (byte) 1); } diff --git a/src/main/java/org/metricshub/ipmi/core/common/PropertiesManager.java b/src/main/java/org/metricshub/ipmi/core/common/PropertiesManager.java index 84384a4..2962707 100644 --- a/src/main/java/org/metricshub/ipmi/core/common/PropertiesManager.java +++ b/src/main/java/org/metricshub/ipmi/core/common/PropertiesManager.java @@ -22,7 +22,10 @@ * ╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱ */ +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + import java.io.IOException; +import java.io.InputStream; import java.util.HashMap; import java.util.Map; import java.util.Properties; @@ -45,6 +48,7 @@ private PropertiesManager() { loadProperties("/vxipmi.properties"); } + @SuppressFBWarnings(value = "MS_EXPOSE_REP", justification = "Singleton: handing out the shared instance is the point") public static PropertiesManager getInstance() { if (instance == null) { instance = new PropertiesManager(); @@ -53,9 +57,9 @@ public static PropertiesManager getInstance() { } private void loadProperties(String name) { - try { + try (InputStream stream = getClass().getResourceAsStream(name)) { Properties props = new Properties(); - props.load(getClass().getResourceAsStream(name)); + props.load(stream); for (Object key : props.keySet()) { this.properties.put(key.toString(), props.getProperty(key.toString())); diff --git a/src/main/java/org/metricshub/ipmi/core/connection/Connection.java b/src/main/java/org/metricshub/ipmi/core/connection/Connection.java index 4f3e81f..6eb1906 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/Connection.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/Connection.java @@ -94,11 +94,11 @@ public class Connection extends TimerTask implements MachineObserver { /** * Time in ms after which a message times out. */ - private int timeout = -1; - private StateMachineAction lastAction; - private int sessionId; - private int managedSystemSessionId; - private byte[] sik; + private volatile int timeout = -1; + private volatile StateMachineAction lastAction; + private volatile int sessionId; + private volatile int managedSystemSessionId; + private volatile byte[] sik; private int handle; diff --git a/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java b/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java index 63ce1e6..c3b8198 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/ConnectionManager.java @@ -34,7 +34,6 @@ import java.net.InetAddress; import java.util.ArrayList; import java.util.List; -import java.util.concurrent.atomic.AtomicInteger; /** * Manages multiple {@link Connection}s @@ -43,7 +42,8 @@ public class ConnectionManager { private Messenger messenger; private List connections; - private static final AtomicInteger SESSIONLESS_TAG = new AtomicInteger(0); + private static final Object SESSIONLESS_TAG_LOCK = new Object(); + private static int sessionlessTag; private static List reservedTags = new ArrayList(); /** @@ -128,34 +128,33 @@ public void close() { * {@link ConnectionManager}. Auto-incremented. */ public static int generateSessionlessTag() { - synchronized (SESSIONLESS_TAG) { + synchronized (SESSIONLESS_TAG_LOCK) { boolean wait = true; // wait(1) clears the interrupt flag when it throws; restore it only once a tag is found, // otherwise every following wait(1) would throw immediately and the loop would hot-spin boolean interrupted = false; while (wait) { - SESSIONLESS_TAG.incrementAndGet(); - SESSIONLESS_TAG.set(SESSIONLESS_TAG.get() % 60); + sessionlessTag = (sessionlessTag + 1) % 60; synchronized (reservedTags) { - if (!reservedTags.contains(SESSIONLESS_TAG.get())) { + if (!reservedTags.contains(sessionlessTag)) { wait = false; } } if (wait) { try { - SESSIONLESS_TAG.wait(1); + SESSIONLESS_TAG_LOCK.wait(1); } catch (InterruptedException e) { interrupted = true; } } } synchronized (reservedTags) { - reservedTags.add(SESSIONLESS_TAG.get()); + reservedTags.add(sessionlessTag); } if (interrupted) { Thread.currentThread().interrupt(); } - return SESSIONLESS_TAG.get(); + return sessionlessTag; } } diff --git a/src/main/java/org/metricshub/ipmi/core/connection/SessionManager.java b/src/main/java/org/metricshub/ipmi/core/connection/SessionManager.java index d076eb2..d74e5d1 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/SessionManager.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/SessionManager.java @@ -34,6 +34,7 @@ import java.net.InetAddress; import java.util.List; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicInteger; /** * Manages multiple {@link Session}s. @@ -42,15 +43,14 @@ public class SessionManager { private static final Logger LOGGER = LoggerFactory.getLogger(SessionManager.class); - private static Integer sessionId = 100; + private static final AtomicInteger SESSION_ID = new AtomicInteger(100); /** * The session ID generated by the {@link SessionManager}. * Auto-incremented. */ - public static synchronized int generateSessionId() { - sessionId %= (Integer.MAX_VALUE / 4); - return sessionId++; + public static int generateSessionId() { + return SESSION_ID.getAndUpdate(id -> (id + 1) % (Integer.MAX_VALUE / 4)); } public static Session establishSession( diff --git a/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java b/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java index 45566b3..1a27f8c 100644 --- a/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java +++ b/src/main/java/org/metricshub/ipmi/core/connection/queue/MessageQueue.java @@ -41,7 +41,7 @@ public class MessageQueue extends TimerTask { private List queue; - private int timeout; + private volatile int timeout; private Timer timer; private Connection connection; private int lastSequenceNumber; diff --git a/src/main/java/org/metricshub/ipmi/core/package-info.java b/src/main/java/org/metricshub/ipmi/core/package-info.java index b985ed2..5010027 100644 --- a/src/main/java/org/metricshub/ipmi/core/package-info.java +++ b/src/main/java/org/metricshub/ipmi/core/package-info.java @@ -4,7 +4,16 @@ * * @see org.metricshub.ipmi.core.api */ +// Applies to all the subpackages. EI_EXPOSE_REP also matches EI_EXPOSE_REP2 +@SuppressFBWarnings(value = { "CT_CONSTRUCTOR_THROW", "EI_EXPOSE_REP" }, justification = "CT_CONSTRUCTOR_THROW: " + + "constructors reject invalid arguments and packets, and no class guards security-sensitive state that a " + + "finalizer attack could exploit. EI_EXPOSE_REP: the protocol core shares buffers, records and " + + "collaborators by reference by design; messages are built and decoded in place, and response data and " + + "records are mutable holders with public setters, so a defensive copy would add allocations without " + + "protecting any invariant") package org.metricshub.ipmi.core; + +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; /*- * ╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲╱╲ * IPMI Java Client diff --git a/src/main/java/org/metricshub/ipmi/core/transport/UdpMessenger.java b/src/main/java/org/metricshub/ipmi/core/transport/UdpMessenger.java index 4c08990..b59b93c 100644 --- a/src/main/java/org/metricshub/ipmi/core/transport/UdpMessenger.java +++ b/src/main/java/org/metricshub/ipmi/core/transport/UdpMessenger.java @@ -45,13 +45,13 @@ public class UdpMessenger extends Thread implements Messenger { private List listeners; - private boolean closing = false; + private volatile boolean closing = false; /** * Size of the message data buffer. Default * {@link UdpMessenger#DEFAULTBUFFERSIZE}. */ - private int bufferSize; + private volatile int bufferSize; private static final int DEFAULTBUFFERSIZE = 512; @@ -89,7 +89,6 @@ public UdpMessenger(int port) throws SocketException, UnknownHostException { * bind to the specified local port. */ public UdpMessenger(int port, InetAddress address) throws SocketException { - sentPackets = 0; this.port = port; listeners = new ArrayList(); bufferSize = DEFAULTBUFFERSIZE; @@ -195,16 +194,6 @@ public void unregister(UdpListener listener) { } } - private static int sentPackets = 0; - - /** - * Returns number of packets sent since last creation of the instance of - * {@link UdpMessenger}. For debug/testing purposes only. - */ - public static int getSentPackets() { - return sentPackets; - } - /** * Sends {@link UdpMessage}. * @@ -225,6 +214,5 @@ public synchronized void send(UdpMessage message) throws IOException { } catch (InterruptedException e) { currentThread().interrupt(); } - ++sentPackets; } } diff --git a/src/site/markdown/configuration.md b/src/site/markdown/configuration.md index 9787595..f3f35ad 100644 --- a/src/site/markdown/configuration.md +++ b/src/site/markdown/configuration.md @@ -50,9 +50,10 @@ chosen by the operating system for each session. ### Credentials `username` and `password` are the IPMI account of the BMC: see -[Preparing the BMC](preparing-the-bmc.html#creating-the-account). The password is a `char[]`; -the library converts it to a `String` internally to open the session and does not clear the -array, so clear it yourself once you no longer need the configuration. +[Preparing the BMC](preparing-the-bmc.html#creating-the-account). Both are sent to the BMC +encoded in UTF-8, whatever the platform charset. The password is a `char[]`; the library converts +it to a `String` internally to open the session and does not clear the array, so clear it +yourself once you no longer need the configuration. The client opens every session with the **User** privilege level, which is enough for every `IpmiClient` method. diff --git a/src/site/markdown/upgrading.md b/src/site/markdown/upgrading.md index 6fc0071..a5a40b0 100644 --- a/src/site/markdown/upgrading.md +++ b/src/site/markdown/upgrading.md @@ -1,4 +1,4 @@ -keywords: upgrade, migration, release notes, breaking changes, protected fields, accessors, abstractsensorrecord, validateresponse, org.sentrysoftware +keywords: upgrade, migration, release notes, breaking changes, protected fields, accessors, abstractsensorrecord, validateresponse, utf-8, bmc key, org.sentrysoftware description: What changes when upgrading the IPMI Java Client — from 1.2.02, from 1.2.01, and from the org.sentrysoftware:ipmi artifact of 1.2.00 and earlier. # Upgrading @@ -13,7 +13,13 @@ The `IpmiClient` API is unchanged, and the client is more tolerant of real-world records (`C0h` – `FFh`) are decoded as `OemRecord`, and any record that cannot be decoded is logged and skipped, where 1.2.02 returned no sensors and no FRUs at all ([OEM and unknown records](supported-commands.html#oem-and-unknown-records)); -* a BMC that answers a whole-record Get SDR with a truncated record is read again in chunks. +* a BMC that answers a whole-record Get SDR with a truncated record is read again in chunks; +* the user name and password are encoded in UTF-8 whatever the platform charset (1.2.02 used the + platform charset, UTF-8 by default only since Java 18), and the + [BMC key](configuration.html#bmc-key) is used as raw bytes: a key with bytes `80h` or above no + longer gets corrupted into a wrong session key; +* the user name is limited to 16 bytes once encoded, as the IPMI specification requires, instead of + 16 characters. Code that **extends** the library's protocol classes needs the changes below. @@ -39,6 +45,15 @@ with the new `protected` accessors: `MessageComposer` are now `final` (they only had private constructors, so they could not be subclassed anyway). +### Credentials and transport + +| Class | Change | +| --- | --- | +| `AuthenticationAlgorithm` | `getKeyExchangeAuthenticationCode(byte[] data, byte[] key)` and `checkKeyExchangeAuthenticationCode(byte[] data, byte[] key, byte[] password)` take the key and the password as bytes instead of a `String` | +| `IntegrityAlgorithm`, `ConfidentialityAesCbc128` | The `protected` constants `CONST1` and `CONST2` are now `private` | +| `UdpMessenger` | `getSentPackets()`, a debug counter, is removed | +| `ProtocolDecoder` | `decodePayload(...)` throws `IllegalArgumentException` on an empty payload instead of a `NullPointerException` | + ### Sensor records `FullSensorRecord`, `CompactSensorRecord` and `EventOnlyRecord` now extend the new diff --git a/src/test/java/org/metricshub/ipmi/core/coding/commands/session/Rakp1Test.java b/src/test/java/org/metricshub/ipmi/core/coding/commands/session/Rakp1Test.java new file mode 100644 index 0000000..16342bc --- /dev/null +++ b/src/test/java/org/metricshub/ipmi/core/coding/commands/session/Rakp1Test.java @@ -0,0 +1,63 @@ +package org.metricshub.ipmi.core.coding.commands.session; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; + +import java.io.ByteArrayOutputStream; +import java.nio.charset.StandardCharsets; +import java.util.Arrays; + +import javax.crypto.Mac; +import javax.crypto.spec.SecretKeySpec; + +import org.junit.jupiter.api.Test; +import org.metricshub.ipmi.core.coding.commands.PrivilegeLevel; +import org.metricshub.ipmi.core.coding.security.CipherSuite; +import org.metricshub.ipmi.core.coding.security.SecurityConstants; + +class Rakp1Test { + + private static final CipherSuite SUITE_3 = new CipherSuite( + (byte) 3, + SecurityConstants.AA_RAKP_HMAC_SHA1, + SecurityConstants.CA_AES_CBC128, + SecurityConstants.IA_HMAC_SHA1_96); + + /** + * SIK = HMAC-SHA1 keyed with Kg (or the password) over Rc | Rm | RoleM | ULengthM | UNameM (IPMI 2.0 section + * 13.31), computed independently of the library. + */ + private static byte[] expectedSik(Rakp1 rakp1, byte[] managedSystemRandom, String username, byte[] key) + throws Exception { + ByteArrayOutputStream base = new ByteArrayOutputStream(); + base.write(rakp1.getConsoleRandomNumber()); + base.write(managedSystemRandom); + base.write(0x12); // User privilege level (2h), name-only lookup (10h) + byte[] name = username.getBytes(StandardCharsets.UTF_8); + base.write(name.length); + base.write(name); + Mac mac = Mac.getInstance("HmacSHA1"); + mac.init(new SecretKeySpec(key, "HmacSHA1")); + return mac.doFinal(base.toByteArray()); + } + + @Test + void calculateSikUsesRawBmcKeyAndUtf8Credentials() throws Exception { + Rakp1ResponseData rakp2 = new Rakp1ResponseData(); + byte[] managedSystemRandom = new byte[16]; + Arrays.fill(managedSystemRandom, (byte) 0xa5); + rakp2.setManagedSystemRandomNumber(managedSystemRandom); + String username = "usér"; + + // Kg bytes of 80h and above used to go through new String(key).getBytes() and come out mangled + byte[] bmcKey = { (byte) 0x80, (byte) 0xff, 0x01, (byte) 0xc3 }; + Rakp1 twoKey = new Rakp1(0x1234, PrivilegeLevel.User, username, "password", bmcKey, SUITE_3); + assertArrayEquals(expectedSik(twoKey, managedSystemRandom, username, bmcKey), twoKey.calculateSik(rakp2)); + + // without Kg, the key is the password, in UTF-8 whatever the platform charset + String password = "pässwörd"; + Rakp1 oneKey = new Rakp1(0x1234, PrivilegeLevel.User, username, password, null, SUITE_3); + assertArrayEquals( + expectedSik(oneKey, managedSystemRandom, username, password.getBytes(StandardCharsets.UTF_8)), + oneKey.calculateSik(rakp2)); + } +}