tomaswolf commented on a change in pull request #198:
URL: https://github.com/apache/mina-sshd/pull/198#discussion_r641321729
##########
File path:
sshd-common/src/main/java/org/apache/sshd/common/config/keys/OpenSshCertificate.java
##########
@@ -35,6 +36,10 @@
int SSH_CERT_TYPE_USER = 1;
int SSH_CERT_TYPE_HOST = 2;
+ long VALID_AFTER_FOREVER_EPOCH = 0L;
+ // note that this is the max value of a uint64 (java.lang.Long.MAX_VALUE
is the max of a *signed* int64)
+ long VALID_BEFORE_FOREVER_EPOCH = 0xffffffffffffffffL;
Review comment:
Either write this as ~0L, or use underscores to make this more readable:
0xffff_ffff_ffff_ffffL is much easier to read, and one immediately sees that
there are indeed 16 'f's.
##########
File path:
sshd-common/src/main/java/org/apache/sshd/common/config/keys/OpenSshCertificate.java
##########
@@ -91,7 +104,83 @@
*/
static boolean isValidNow(OpenSshCertificate cert) {
long now = TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis());
- return Long.compareUnsigned(cert.getValidAfter(), now) <= 0
- && Long.compareUnsigned(now, cert.getValidBefore()) < 0;
+ return Long.compareUnsigned(cert.getValidAfterEpochSeconds(), now) <= 0
+ && Long.compareUnsigned(now,
cert.getValidBeforeEpochSeconds()) < 0;
+ }
+
+ /**
+ * According to <A HREF=
+ *
"https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.certkeys#L222-L262";>PROTOCOL.certkeys</A>:
+ *
+ * Critical Options is a set of bytes that is
+ *
+ * [overall length][name(string)][data(string)]...
+ *
+ * Where each Critical Option is encoded as a name (string) and data
(string)
+ *
+ * Then the entire name + data strings are added as bytes (which will get
a length prefix)
+ */
+ class CriticalOption {
+
+ private String name;
+ private String data;
+
+ public CriticalOption(String name, String data) {
+ this.name = name;
+ this.data = data;
+ }
+
+ public CriticalOption(String name) {
+ this.name = name;
+ }
+
+ public String getName() {
+ return name;
+ }
+
+ public void setName(String name) {
+ this.name = name;
+ }
+
+ public String getData() {
+ return data;
+ }
+
+ public void setData(String data) {
+ this.data = data;
+ }
+
+ @Override
+ public String toString() {
+ return "CriticalOption{" +
+ "name='" + name + '\'' +
+ ", data='" + data + '\'' +
+ '}';
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+
+ CriticalOption that = (CriticalOption) o;
+
+ if (name != null ? !name.equals(that.name) : that.name != null) {
+ return false;
+ }
+ return data != null ? data.equals(that.data) : that.data == null;
Review comment:
1. Using Objects.equals() and Objects.hash() in hashCode() below would
simplify this; no need to check for null.
2. equals and hashCode based on non-final fields is not a good idea. If a
CriticalOption object is stored in a HashMap and then name or data is modified,
you'll never find it again.
I'd make the fields final and drop the setters.
##########
File path:
sshd-common/src/main/java/org/apache/sshd/common/config/keys/OpenSshCertificate.java
##########
@@ -52,26 +57,34 @@
Collection<String> getPrincipals();
/**
- * Retrieves the time in number of seconds since the {@link
java.time.Instant#EPOCH} at which this certificate
- * becomes or became valid.
- *
- * @return the number of seconds since the Instant.EPOCH <em>as an
unsigned 64bit value</em>
- * @see {{@link #isValidNow(OpenSshCertificate)}
+ * When null, implies forever
*/
- long getValidAfter();
+ Instant getValidAfter();
+
+ default long getValidAfterEpochSeconds() {
+ if (getValidAfter() == null) {
+ return VALID_AFTER_FOREVER_EPOCH;
+ }
+ return getValidAfter().getEpochSecond();
Review comment:
I don't understand why you handle validAfter == 0 specially. That would
be Instant.EPOCH.
I think you can handle all values in the range 0 .. Long.MAX_VALUE normally.
If it's greater, the signed value will appear as < 0. Here I think handling the
~0L case ("forever") as null is fine; all others I'd map to Long.MAX_VALUE.
##########
File path:
sshd-common/src/main/java/org/apache/sshd/common/config/keys/OpenSshCertificateImpl.java
##########
@@ -229,8 +228,81 @@ public String toString() {
+ "[id=" + getId()
+ ", serial=" + getSerial()
+ ", type=" + getType()
- + ", validAfter=" + toDate(getValidAfter())
- + ", validBefore=" + toDate(getValidBefore())
+ + ", validAfter=" + toStringDate(getValidAfter())
+ + ", validBefore=" + toStringDate(getValidBefore())
+ "]";
}
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+
+ OpenSshCertificateImpl that = (OpenSshCertificateImpl) o;
+
+ if (serial != that.serial) {
+ return false;
+ }
+ if (type != that.type) {
+ return false;
+ }
+ if (keyType != null ? !keyType.equals(that.keyType) : that.keyType !=
null) {
+ return false;
+ }
+ if (!Arrays.equals(nonce, that.nonce)) {
+ return false;
+ }
+ if (certificatePublicKey != null
+ ? !certificatePublicKey.equals(that.certificatePublicKey) :
that.certificatePublicKey != null) {
+ return false;
+ }
+ if (id != null ? !id.equals(that.id) : that.id != null) {
+ return false;
+ }
+ if (principals != null ? !principals.equals(that.principals) :
that.principals != null) {
+ return false;
+ }
+ if (validAfter != null ? !validAfter.equals(that.validAfter) :
that.validAfter != null) {
+ return false;
+ }
+ if (validBefore != null ? !validBefore.equals(that.validBefore) :
that.validBefore != null) {
+ return false;
+ }
+ if (criticalOptions != null ?
!criticalOptions.equals(that.criticalOptions) : that.criticalOptions != null) {
+ return false;
+ }
+ if (extensions != null ? !extensions.equals(that.extensions) :
that.extensions != null) {
+ return false;
+ }
+ if (reserved != null ? !reserved.equals(that.reserved) : that.reserved
!= null) {
+ return false;
+ }
+ if (caPubKey != null ? !caPubKey.equals(that.caPubKey) : that.caPubKey
!= null) {
+ return false;
+ }
+ return Arrays.equals(signature, that.signature);
+ }
+
+ @Override
+ public int hashCode() {
Review comment:
Again: hashCode() on non-final fields.
With non-final fields I think this should not have content-based equals() or
hashCode(). As soon as clients can modify fields, this is asking for trouble.
##########
File path:
sshd-common/src/main/java/org/apache/sshd/common/config/keys/KeyUtils.java
##########
@@ -47,21 +47,8 @@
import java.security.spec.ECParameterSpec;
import java.security.spec.InvalidKeySpecException;
import java.security.spec.RSAPublicKeySpec;
+import java.util.*;
Review comment:
I don't think Apache MINA sshd uses wildcard imports?
##########
File path:
sshd-common/src/main/java/org/apache/sshd/common/config/keys/OpenSshCertificate.java
##########
@@ -52,26 +57,34 @@
Collection<String> getPrincipals();
/**
- * Retrieves the time in number of seconds since the {@link
java.time.Instant#EPOCH} at which this certificate
- * becomes or became valid.
- *
- * @return the number of seconds since the Instant.EPOCH <em>as an
unsigned 64bit value</em>
- * @see {{@link #isValidNow(OpenSshCertificate)}
+ * When null, implies forever
*/
- long getValidAfter();
+ Instant getValidAfter();
+
+ default long getValidAfterEpochSeconds() {
+ if (getValidAfter() == null) {
+ return VALID_AFTER_FOREVER_EPOCH;
+ }
+ return getValidAfter().getEpochSecond();
+ }
/**
- * Retrieves the time in number of seconds since the {@link
java.time.Instant#EPOCH} at which this certificate
- * becomes or became invalid.
+ * When null, implies forever
*
- * @return the number of seconds since the Instant.EPOCH <em>as an
unsigned 64bit value</em>
- * @see {{@link #isValidNow(OpenSshCertificate)}
+ * @return
*/
Review comment:
Why strip down the javadoc?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
