You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by GitBox <gi...@apache.org> on 2022/05/10 15:13:24 UTC
[GitHub] [mina-sshd] vukzeka opened a new pull request, #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
vukzeka opened a new pull request, #222:
URL: https://github.com/apache/mina-sshd/pull/222
- fixed bug with CertificateOption data encoding
- Improved CertificateOption docs
- Added critical option ordering and duplicate check
- updated tests to include critical options
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] vukzeka commented on a diff in pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
vukzeka commented on code in PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#discussion_r870194796
##########
sshd-core/src/main/java/org/apache/sshd/certificate/OpenSshCertificateBuilder.java:
##########
@@ -104,7 +110,21 @@ public OpenSshCertificateBuilder principals(Collection<String> principals) {
}
public OpenSshCertificateBuilder criticalOptions(List<OpenSshCertificate.CertificateOption> criticalOptions) {
- this.criticalOptions = criticalOptions;
+ if (criticalOptions != null && !criticalOptions.isEmpty()) {
+ // check if any duplicates
+ Set<String> names = new HashSet<String>();
+ Set<String> duplicates = criticalOptions.stream().filter(option -> !names.add(option.getName()))
+ .map(option -> option.getName())
+ .collect(Collectors.toSet());
+ if (!duplicates.isEmpty()) {
+ throw new IllegalArgumentException("Duplicate critical option: " + String.join(",", duplicates));
+ }
+ // lexically order by "name"
+ List<OpenSshCertificate.CertificateOption> sortedCriticalOptions = criticalOptions.stream()
+ .sorted(Comparator.comparing(OpenSshCertificate.CertificateOption::getName))
+ .collect(Collectors.toList());
+ this.criticalOptions = sortedCriticalOptions;
+ }
Review Comment:
@tomaswolf I was not aware that duplicate and sorting are applied for extensions.
I just checked with ssh-keygen and is possible to add custom extensions and these are not sorted and also duplicates are allowed
for example following
```
ssh-keygen -s user_ca -I certN -n user -O extension:test=value -O extension:test=value -O extension:b@b.com=value -O extension:a@a.com=value -O critical:b_critical_option=value -O critical:a_critical_option=value -V +10d user-key.pub
```
would generate
```Type: ssh-rsa-cert-v01@openssh.com user certificate
Public key: RSA-CERT SHA256:c6uq2hsYBCROmuFrWz38sIk+0Hlx75l5kOqRo7J1wv8
Signing CA: RSA SHA256:qBnc5EM9d1dlZSfbtnPBh45cAJt5jM7LcLeGDbFOd38 (using rsa-sha2-512)
Key ID: "certN"
Serial: 0
Valid: from 2022-05-11T13:24:00 to 2022-05-21T13:25:15
Principals:
user
Critical Options: (none)
Extensions:
permit-X11-forwarding
permit-agent-forwarding
permit-port-forwarding
permit-pty
permit-user-rc
test UNKNOWN OPTION (len 9)
test UNKNOWN OPTION (len 9)
b@b.com UNKNOWN OPTION (len 9)
a@a.com UNKNOWN OPTION (len 9)
```
So it looks to me
- duplicates are checked only for standard extensions
- ordering is applied only for standard extensions
- all custom extensions are added in the order as specified and custom duplicates are allowed
@tomaswolf I can add like that if that sounds correct?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a diff in pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on code in PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#discussion_r869427309
##########
sshd-common/src/main/java/org/apache/sshd/common/util/buffer/Buffer.java:
##########
@@ -348,29 +348,36 @@ public List<OpenSshCertificate.CertificateOption> getCertificateOptions() {
/**
* According to <A HREF=
- * "https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.certkeys#L222-L262">PROTOCOL.certkeys</A>:
+ * "https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.certkeys#L221-L319">PROTOCOL.certkeys</A>:
*
* Critical Options is a set of bytes that is
*
- * [overall length][name(string)][data(string)]...
+ * [overall length][name(string)][[length of buffer][[length of string][data(string)]]]...
*
- * Where each Critical Option is encoded as a name (string) and data (string)
+ * Where each Certificate Option is encoded as a name (string) and buffer of data (string packed in a buffer)
*
- * Then the entire name + data strings are added as bytes (which will get a length prefix)
+ * Then the entire name (string) + data (buffer) are added as bytes (which will get a length prefix)
*
* @param charset {@link Charset} to use for converting bytes to characters
* @return the parsed result, never {@code null}, but possibly empty
*/
public List<OpenSshCertificate.CertificateOption> getCertificateOptions(Charset charset) {
- // pull out entire Critical Options section
- final ByteArrayBuffer optionBuffer = new ByteArrayBuffer(getBytes());
-
List<OpenSshCertificate.CertificateOption> list = new ArrayList<>();
- while (optionBuffer.available() > 0) {
- String name = optionBuffer.getString(charset);
- String data = GenericUtils.trimToEmpty(optionBuffer.getString(charset));
- list.add(new OpenSshCertificate.CertificateOption(name, data.length() > 0 ? data : null));
+ if (available() > 0) {
+ // pull out entire Certificate Options section
+ final ByteArrayBuffer optionBuffer = new ByteArrayBuffer(getBytes());
Review Comment:
No need for *final* keyword
##########
sshd-common/src/main/java/org/apache/sshd/common/util/buffer/Buffer.java:
##########
@@ -348,29 +348,36 @@ public List<OpenSshCertificate.CertificateOption> getCertificateOptions() {
/**
* According to <A HREF=
- * "https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.certkeys#L222-L262">PROTOCOL.certkeys</A>:
+ * "https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.certkeys#L221-L319">PROTOCOL.certkeys</A>:
*
* Critical Options is a set of bytes that is
*
- * [overall length][name(string)][data(string)]...
+ * [overall length][name(string)][[length of buffer][[length of string][data(string)]]]...
*
- * Where each Critical Option is encoded as a name (string) and data (string)
+ * Where each Certificate Option is encoded as a name (string) and buffer of data (string packed in a buffer)
*
- * Then the entire name + data strings are added as bytes (which will get a length prefix)
+ * Then the entire name (string) + data (buffer) are added as bytes (which will get a length prefix)
*
* @param charset {@link Charset} to use for converting bytes to characters
* @return the parsed result, never {@code null}, but possibly empty
*/
public List<OpenSshCertificate.CertificateOption> getCertificateOptions(Charset charset) {
- // pull out entire Critical Options section
- final ByteArrayBuffer optionBuffer = new ByteArrayBuffer(getBytes());
-
List<OpenSshCertificate.CertificateOption> list = new ArrayList<>();
- while (optionBuffer.available() > 0) {
- String name = optionBuffer.getString(charset);
- String data = GenericUtils.trimToEmpty(optionBuffer.getString(charset));
- list.add(new OpenSshCertificate.CertificateOption(name, data.length() > 0 ? data : null));
+ if (available() > 0) {
+ // pull out entire Certificate Options section
+ final ByteArrayBuffer optionBuffer = new ByteArrayBuffer(getBytes());
Review Comment:
Why can't it be `Buffer optionBuffer = new ...` - are you using any *ByteArrayBuffer* methods that are not available via the *Buffer* class ?
Hint: recommend to use the "weakest" variable type you need and not what you know its full type is.
##########
sshd-common/src/main/java/org/apache/sshd/common/util/buffer/Buffer.java:
##########
@@ -348,29 +348,36 @@ public List<OpenSshCertificate.CertificateOption> getCertificateOptions() {
/**
* According to <A HREF=
- * "https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.certkeys#L222-L262">PROTOCOL.certkeys</A>:
+ * "https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.certkeys#L221-L319">PROTOCOL.certkeys</A>:
*
* Critical Options is a set of bytes that is
*
- * [overall length][name(string)][data(string)]...
+ * [overall length][name(string)][[length of buffer][[length of string][data(string)]]]...
*
- * Where each Critical Option is encoded as a name (string) and data (string)
+ * Where each Certificate Option is encoded as a name (string) and buffer of data (string packed in a buffer)
*
- * Then the entire name + data strings are added as bytes (which will get a length prefix)
+ * Then the entire name (string) + data (buffer) are added as bytes (which will get a length prefix)
*
* @param charset {@link Charset} to use for converting bytes to characters
* @return the parsed result, never {@code null}, but possibly empty
*/
public List<OpenSshCertificate.CertificateOption> getCertificateOptions(Charset charset) {
- // pull out entire Critical Options section
- final ByteArrayBuffer optionBuffer = new ByteArrayBuffer(getBytes());
-
List<OpenSshCertificate.CertificateOption> list = new ArrayList<>();
- while (optionBuffer.available() > 0) {
- String name = optionBuffer.getString(charset);
- String data = GenericUtils.trimToEmpty(optionBuffer.getString(charset));
- list.add(new OpenSshCertificate.CertificateOption(name, data.length() > 0 ? data : null));
+ if (available() > 0) {
+ // pull out entire Certificate Options section
+ final ByteArrayBuffer optionBuffer = new ByteArrayBuffer(getBytes());
+
+ while (optionBuffer.available() > 0) {
+ String name = optionBuffer.getString(charset);
+ String data = null;
+ final ByteArrayBuffer dataBuffer = new ByteArrayBuffer(optionBuffer.getBytes());
Review Comment:
Same remarks as before (including no need for *final*)
##########
sshd-core/src/main/java/org/apache/sshd/certificate/OpenSshCertificateBuilder.java:
##########
@@ -104,7 +110,21 @@ public OpenSshCertificateBuilder principals(Collection<String> principals) {
}
public OpenSshCertificateBuilder criticalOptions(List<OpenSshCertificate.CertificateOption> criticalOptions) {
- this.criticalOptions = criticalOptions;
+ if (criticalOptions != null && !criticalOptions.isEmpty()) {
+ // check if any duplicates
+ Set<String> names = new HashSet<String>();
Review Comment:
No need to specify type of *HashSet* use diamond...
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on a diff in pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on code in PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#discussion_r870397695
##########
sshd-core/src/main/java/org/apache/sshd/certificate/OpenSshCertificateBuilder.java:
##########
@@ -104,7 +110,21 @@ public OpenSshCertificateBuilder principals(Collection<String> principals) {
}
public OpenSshCertificateBuilder criticalOptions(List<OpenSshCertificate.CertificateOption> criticalOptions) {
- this.criticalOptions = criticalOptions;
+ if (criticalOptions != null && !criticalOptions.isEmpty()) {
+ // check if any duplicates
+ Set<String> names = new HashSet<String>();
+ Set<String> duplicates = criticalOptions.stream().filter(option -> !names.add(option.getName()))
+ .map(option -> option.getName())
+ .collect(Collectors.toSet());
+ if (!duplicates.isEmpty()) {
+ throw new IllegalArgumentException("Duplicate critical option: " + String.join(",", duplicates));
+ }
+ // lexically order by "name"
+ List<OpenSshCertificate.CertificateOption> sortedCriticalOptions = criticalOptions.stream()
+ .sorted(Comparator.comparing(OpenSshCertificate.CertificateOption::getName))
+ .collect(Collectors.toList());
+ this.criticalOptions = sortedCriticalOptions;
+ }
Review Comment:
I based my comment of the [OpenSSH documentation](https://github.com/openssh/openssh-portable/blob/0086a286ea6bbd11ca9b664ac3bb12b27443d6eb/PROTOCOL.certkeys#L274):
> The encoding and ordering of extensions in this field is identical to that of the critical options, as is the requirement that each name appear only once.
I had not checked what the OpenSSH code actually does. It appears that [auth-options.c](https://github.com/openssh/openssh-portable/blob/0086a286ea6bbd11ca9b664ac3bb12b27443d6eb/auth-options.c#L72) does not enforce _any_ ordering or key uniqueness requirements. [ssh-keygen.c](https://github.com/openssh/openssh-portable/blob/0086a286ea6bbd11ca9b664ac3bb12b27443d6eb/ssh-keygen.c#L1601) appears to only sort, but not check for duplicates, [except](https://github.com/openssh/openssh-portable/blob/0086a286ea6bbd11ca9b664ac3bb12b27443d6eb/ssh-keygen.c#L1964) for {{force-command}} and {{source-address}}.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on a diff in pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on code in PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#discussion_r870072991
##########
sshd-core/src/test/java/org/apache/sshd/certificates/GenerateOpenSSHClientCertificateTest.java:
##########
@@ -154,7 +150,12 @@ public void signCertificate() throws Exception {
.publicKey(clientPublicKey)
.id("user01")
.principals(Collections.singletonList("user01"))
+ .criticalOptions(Arrays.asList(
+ new OpenSshCertificate.CertificateOption("force-command", "/path/to/script.sh"),
+ new OpenSshCertificate.CertificateOption("source-address", "127.0.0.1/32"),
+ new OpenSshCertificate.CertificateOption("verify-required")))
Review Comment:
Just regenerate the certs with OpenSSH and replace the files in test/resources. The Java code shows how they would need to be regenerated to match.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] alex-sherwin commented on pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
alex-sherwin commented on PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#issuecomment-1124459535
@vukzeka I just pushed a commit https://github.com/alex-sherwin/mina-sshd/commit/f5966b48e3ac2ab375269aae553c765cf62488cf
It shows how to use testcontainers to build an image w/ ssh-keygen and:
1. How you can use **ssh-keygen** to parse/view a OpenSSH Certificate file and check the exit code
2. How you can use **ssh-keygen** to generate data and copy the contents back to Java (this one is not really that useful for what we're discussing here, I just created it to see how it would be done)
This test is *not* using the testcontainers JUnit automatic test lifecycle stuff (where you define the `GenericContainer` as a class-level property or static prop with `@ClassRule` annotations to manage the container lifecycle).
This is because all that stuff is geared towards long-running services, etc.
Instead, the unit test itself is defining the docker image + container, starting it, and using a custom wait strategy (which checks for exit, but does not assert the exit code itself), so that the unit test function can inspect the container exit code and logs and apply junit assertions to them.
If you do add a test using this kind of technique, you should probably set it up to be a parameterized test like **GenerateOpenSSHClientCertificateTest** with all the cert type variations
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] alex-sherwin commented on pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
alex-sherwin commented on PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#issuecomment-1122693618
As noted on https://issues.apache.org/jira/browse/SSHD-1266
The original PR's for the OpenSSH certificate support brought in support for Testcontainers based Docker unit testing
Perhaps this PR should include a new unit test which uses Testcontainers/Docker to use the real OpenSSH *ssh-keygen* binary to parse the OpenSSH Certificate output from MINA
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#issuecomment-1125229695
Done
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#issuecomment-1122660236
@lgoldstein : please downgrade then formatter maven plug-in again. (Revert commit 9f8aa3dba.) It doesn't run with Java 1.8.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] vukzeka commented on a diff in pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
vukzeka commented on code in PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#discussion_r871096735
##########
sshd-core/src/main/java/org/apache/sshd/certificate/OpenSshCertificateBuilder.java:
##########
@@ -267,4 +275,38 @@ public OpenSshCertificate sign(KeyPair caKeypair, String signatureAlgorithm) thr
return cert;
}
+
+ /**
+ * Validate if certificate options are correct
+ *
+ * @param options Options
+ */
+ private void validateOptions(List<OpenSshCertificate.CertificateOption> options) {
+ if (options != null && !options.isEmpty()) {
+ // check if any duplicates
+ Set<String> names = new HashSet<>();
+ Set<String> duplicates = options.stream().filter(option -> !names.add(option.getName()))
+ .map(option -> option.getName())
Review Comment:
Done
##########
sshd-core/src/main/java/org/apache/sshd/certificate/OpenSshCertificateBuilder.java:
##########
@@ -267,4 +275,38 @@ public OpenSshCertificate sign(KeyPair caKeypair, String signatureAlgorithm) thr
return cert;
}
+
+ /**
+ * Validate if certificate options are correct
+ *
+ * @param options Options
+ */
+ private void validateOptions(List<OpenSshCertificate.CertificateOption> options) {
+ if (options != null && !options.isEmpty()) {
+ // check if any duplicates
+ Set<String> names = new HashSet<>();
+ Set<String> duplicates = options.stream().filter(option -> !names.add(option.getName()))
+ .map(option -> option.getName())
+ .collect(Collectors.toSet());
+ if (!duplicates.isEmpty()) {
+ throw new IllegalArgumentException("Duplicate option: " + String.join(",", duplicates));
Review Comment:
Done
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] vukzeka commented on pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
vukzeka commented on PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#issuecomment-1124693099
> @vukzeka I just pushed a commit [alex-sherwin@f5966b4](https://github.com/alex-sherwin/mina-sshd/commit/f5966b48e3ac2ab375269aae553c765cf62488cf)
>
> It shows how to use testcontainers to build an image w/ ssh-keygen and:
>
> 1. How you can use **ssh-keygen** to parse/view a OpenSSH Certificate file and check the exit code
> 2. How you can use **ssh-keygen** to generate data and copy the contents back to Java
>
> This test is _not_ using the testcontainers JUnit automatic test lifecycle stuff (where you define the `GenericContainer` as a class-level property or static prop with `@ClassRule` annotations to manage the container lifecycle, but, testcontainers will still auto cleanup/reap the container and images after JVM exit automagically).
>
> This is because all that stuff is geared towards long-running daemon services, etc.
>
> Instead, the unit test itself is defining the docker image + container, starting it, and using a custom wait strategy (which checks for exit, but does not assert the exit code itself), so that the unit test function can inspect the container exit code and logs and apply junit assertions to them.
>
> If you do add a test using this kind of technique, you should probably set it up to be a parameterized test like **GenerateOpenSSHClientCertificateTest** with all the cert type variations
Great, thanks @alex-sherwin
[This](https://github.com/alex-sherwin/mina-sshd/commit/f5966b48e3ac2ab375269aae553c765cf62488cf#diff-87ff30dcfbfe5636d9eb3413bb6530396eeea5409eae18163ac9561951bcedf2R113) and [this](https://github.com/alex-sherwin/mina-sshd/commit/f5966b48e3ac2ab375269aae553c765cf62488cf#diff-87ff30dcfbfe5636d9eb3413bb6530396eeea5409eae18163ac9561951bcedf2R137-R144) is exactly what i was looking for.
Something like that would help to not store pre-generated certs on the repo already test can
- generate keys using ssh-keygen
- generate certs using ssh-keygen
- parse them using code
- generate certs from code
- and validate certs back with ssh-keygen
and more important will be usable also for some other tests not related to this functionality.
I will create some example later this week but will be separate from this PR.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] vukzeka commented on pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
vukzeka commented on PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#issuecomment-1124402694
> As I noted on https://issues.apache.org/jira/browse/SSHD-1266
>
> The original PR's for the OpenSSH certificate support brought in support for Testcontainers based Docker unit testing
>
> Perhaps this PR should include a new unit test which uses Testcontainers/Docker to use the real OpenSSH _ssh-keygen_ binary to parse the OpenSSH Certificate output from MINA?
>
> It could minimally use _ssh-keygen_ to parse the MINA generated OpenSSH Certificate and check for a zero exit code
Hi @alex-sherwin
It was not in the "scope" but I was thinking same. Not only for this use case but in general something like that would be useful.
I just saw test you were referring and I'm thinking that probably easiest would be to have image with **ssh-keygen**, something like
```
FROM alpine:3.15.4
RUN apk update && \
apk add --no-cache openssh-keygen bash && \
rm -rf /var/cache/apk/*
CMD ["ssh-keygen"]
```
and then run same as **OneShotStartupCheckStrategy** supplying different commands
`ssh-keygen COMMAND`
But question if you know, is there easy way to capture different outputs of interest (generated files, exit code, stdout, stderror, etc.) with such approach?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] vukzeka commented on a diff in pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
vukzeka commented on code in PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#discussion_r870062043
##########
sshd-common/src/main/java/org/apache/sshd/common/util/buffer/Buffer.java:
##########
@@ -348,29 +348,36 @@ public List<OpenSshCertificate.CertificateOption> getCertificateOptions() {
/**
* According to <A HREF=
- * "https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.certkeys#L222-L262">PROTOCOL.certkeys</A>:
+ * "https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.certkeys#L221-L319">PROTOCOL.certkeys</A>:
*
* Critical Options is a set of bytes that is
*
- * [overall length][name(string)][data(string)]...
+ * [overall length][name(string)][[length of buffer][[length of string][data(string)]]]...
*
- * Where each Critical Option is encoded as a name (string) and data (string)
+ * Where each Certificate Option is encoded as a name (string) and buffer of data (string packed in a buffer)
*
- * Then the entire name + data strings are added as bytes (which will get a length prefix)
+ * Then the entire name (string) + data (buffer) are added as bytes (which will get a length prefix)
*
* @param charset {@link Charset} to use for converting bytes to characters
* @return the parsed result, never {@code null}, but possibly empty
*/
public List<OpenSshCertificate.CertificateOption> getCertificateOptions(Charset charset) {
- // pull out entire Critical Options section
- final ByteArrayBuffer optionBuffer = new ByteArrayBuffer(getBytes());
-
List<OpenSshCertificate.CertificateOption> list = new ArrayList<>();
- while (optionBuffer.available() > 0) {
- String name = optionBuffer.getString(charset);
- String data = GenericUtils.trimToEmpty(optionBuffer.getString(charset));
- list.add(new OpenSshCertificate.CertificateOption(name, data.length() > 0 ? data : null));
+ if (available() > 0) {
+ // pull out entire Certificate Options section
+ final ByteArrayBuffer optionBuffer = new ByteArrayBuffer(getBytes());
Review Comment:
@lgoldstein same as previous comment
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] vukzeka commented on pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
vukzeka commented on PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#issuecomment-1124693963
> Looks good, some minor comments.
>
> I notice that the tests add the options already in the correct order. Now that we do sort I'd suggest to change the tests to add them in a wrong order.
>
> If you want to do a testcontainers test, I suggest to do a new PR once this one is merged.
Hope is ok now.
Yes, I will add also one test for ssh-keygen later this week and that will be separate PR.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on a diff in pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on code in PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#discussion_r871043739
##########
sshd-core/src/main/java/org/apache/sshd/certificate/OpenSshCertificateBuilder.java:
##########
@@ -267,4 +275,38 @@ public OpenSshCertificate sign(KeyPair caKeypair, String signatureAlgorithm) thr
return cert;
}
+
+ /**
+ * Validate if certificate options are correct
+ *
+ * @param options Options
+ */
+ private void validateOptions(List<OpenSshCertificate.CertificateOption> options) {
+ if (options != null && !options.isEmpty()) {
+ // check if any duplicates
+ Set<String> names = new HashSet<>();
+ Set<String> duplicates = options.stream().filter(option -> !names.add(option.getName()))
+ .map(option -> option.getName())
+ .collect(Collectors.toSet());
+ if (!duplicates.isEmpty()) {
+ throw new IllegalArgumentException("Duplicate option: " + String.join(",", duplicates));
+ }
+ }
+ }
+
+ /**
+ * Lexically order certificate options by "name"
+ *
+ * @param options Options
+ * @return Lexically ordered options
+ */
+ private List<OpenSshCertificate.CertificateOption> lexicallyOrderOptions(
+ List<OpenSshCertificate.CertificateOption> options) {
+ if (options != null && !options.isEmpty()) {
+ return options.stream()
+ .sorted(Comparator.comparing(OpenSshCertificate.CertificateOption::getName))
+ .collect(Collectors.toList());
+ }
+ return options;
Review Comment:
I would return here `Collections.emptyList()`. That would ensure that the builder stores a copy of the parameter the user passed in, and that subsequent modifications of the original list can have no effect on the builder.
##########
sshd-core/src/main/java/org/apache/sshd/certificate/OpenSshCertificateBuilder.java:
##########
@@ -267,4 +275,38 @@ public OpenSshCertificate sign(KeyPair caKeypair, String signatureAlgorithm) thr
return cert;
}
+
+ /**
+ * Validate if certificate options are correct
+ *
+ * @param options Options
+ */
+ private void validateOptions(List<OpenSshCertificate.CertificateOption> options) {
+ if (options != null && !options.isEmpty()) {
+ // check if any duplicates
+ Set<String> names = new HashSet<>();
+ Set<String> duplicates = options.stream().filter(option -> !names.add(option.getName()))
+ .map(option -> option.getName())
Review Comment:
Why not `.map(OpenSshCertificate.CertificateOption::getName)`?
##########
sshd-core/src/main/java/org/apache/sshd/certificate/OpenSshCertificateBuilder.java:
##########
@@ -267,4 +275,38 @@ public OpenSshCertificate sign(KeyPair caKeypair, String signatureAlgorithm) thr
return cert;
}
+
+ /**
+ * Validate if certificate options are correct
+ *
+ * @param options Options
+ */
+ private void validateOptions(List<OpenSshCertificate.CertificateOption> options) {
+ if (options != null && !options.isEmpty()) {
+ // check if any duplicates
+ Set<String> names = new HashSet<>();
+ Set<String> duplicates = options.stream().filter(option -> !names.add(option.getName()))
+ .map(option -> option.getName())
+ .collect(Collectors.toSet());
+ if (!duplicates.isEmpty()) {
+ throw new IllegalArgumentException("Duplicate option: " + String.join(",", duplicates));
Review Comment:
`Set.toString()` will enumerate the content. So perhaps just `+ duplicates` instead of the `String.join(...)`?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on a diff in pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on code in PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#discussion_r869384180
##########
sshd-core/src/main/java/org/apache/sshd/certificate/OpenSshCertificateBuilder.java:
##########
@@ -104,7 +110,21 @@ public OpenSshCertificateBuilder principals(Collection<String> principals) {
}
public OpenSshCertificateBuilder criticalOptions(List<OpenSshCertificate.CertificateOption> criticalOptions) {
- this.criticalOptions = criticalOptions;
+ if (criticalOptions != null && !criticalOptions.isEmpty()) {
+ // check if any duplicates
+ Set<String> names = new HashSet<String>();
+ Set<String> duplicates = criticalOptions.stream().filter(option -> !names.add(option.getName()))
+ .map(option -> option.getName())
+ .collect(Collectors.toSet());
+ if (!duplicates.isEmpty()) {
+ throw new IllegalArgumentException("Duplicate critical option: " + String.join(",", duplicates));
+ }
+ // lexically order by "name"
+ List<OpenSshCertificate.CertificateOption> sortedCriticalOptions = criticalOptions.stream()
+ .sorted(Comparator.comparing(OpenSshCertificate.CertificateOption::getName))
+ .collect(Collectors.toList());
+ this.criticalOptions = sortedCriticalOptions;
+ }
Review Comment:
Thanks for adding this; I also noticed that. The same duplicate detection and sorting is also needed for the extensions.
##########
sshd-core/src/test/java/org/apache/sshd/certificates/GenerateOpenSSHClientCertificateTest.java:
##########
@@ -154,7 +150,12 @@ public void signCertificate() throws Exception {
.publicKey(clientPublicKey)
.id("user01")
.principals(Collections.singletonList("user01"))
+ .criticalOptions(Arrays.asList(
+ new OpenSshCertificate.CertificateOption("force-command", "/path/to/script.sh"),
+ new OpenSshCertificate.CertificateOption("source-address", "127.0.0.1/32"),
+ new OpenSshCertificate.CertificateOption("verify-required")))
Review Comment:
Please also add critical options in the `GenerateOpenSshClientCertificateOracleTest`. That one tests against pre-existing certificates generated using OpenSSH.
##########
sshd-core/src/test/java/org/apache/sshd/certificates/GenerateOpenSSHClientCertificateTest.java:
##########
@@ -154,7 +150,12 @@ public void signCertificate() throws Exception {
.publicKey(clientPublicKey)
.id("user01")
.principals(Collections.singletonList("user01"))
+ .criticalOptions(Arrays.asList(
+ new OpenSshCertificate.CertificateOption("force-command", "/path/to/script.sh"),
+ new OpenSshCertificate.CertificateOption("source-address", "127.0.0.1/32"),
+ new OpenSshCertificate.CertificateOption("verify-required")))
.extensions(Arrays.asList(
+ new OpenSshCertificate.CertificateOption("no-touch-required"),
Review Comment:
A bit off-topic, but I wonder: should non-sk keys even accept this option?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on a diff in pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on code in PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#discussion_r870480760
##########
sshd-core/src/main/java/org/apache/sshd/certificate/OpenSshCertificateBuilder.java:
##########
@@ -104,7 +110,21 @@ public OpenSshCertificateBuilder principals(Collection<String> principals) {
}
public OpenSshCertificateBuilder criticalOptions(List<OpenSshCertificate.CertificateOption> criticalOptions) {
- this.criticalOptions = criticalOptions;
+ if (criticalOptions != null && !criticalOptions.isEmpty()) {
+ // check if any duplicates
+ Set<String> names = new HashSet<String>();
+ Set<String> duplicates = criticalOptions.stream().filter(option -> !names.add(option.getName()))
+ .map(option -> option.getName())
+ .collect(Collectors.toSet());
+ if (!duplicates.isEmpty()) {
+ throw new IllegalArgumentException("Duplicate critical option: " + String.join(",", duplicates));
+ }
+ // lexically order by "name"
+ List<OpenSshCertificate.CertificateOption> sortedCriticalOptions = criticalOptions.stream()
+ .sorted(Comparator.comparing(OpenSshCertificate.CertificateOption::getName))
+ .collect(Collectors.toList());
+ this.criticalOptions = sortedCriticalOptions;
+ }
Review Comment:
Please see OpenSSH [bug 3198](https://bugzilla.mindrot.org/show_bug.cgi?id=3198), fixed in OpenSSH 8.4 via [this commit](https://github.com/openssh/openssh-portable/commit/2d8a3b7e8b0408dfeb933ac5cfd3a58f5bac49af).
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] vukzeka commented on a diff in pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
vukzeka commented on code in PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#discussion_r870774200
##########
sshd-core/src/main/java/org/apache/sshd/certificate/OpenSshCertificateBuilder.java:
##########
@@ -104,7 +110,21 @@ public OpenSshCertificateBuilder principals(Collection<String> principals) {
}
public OpenSshCertificateBuilder criticalOptions(List<OpenSshCertificate.CertificateOption> criticalOptions) {
- this.criticalOptions = criticalOptions;
+ if (criticalOptions != null && !criticalOptions.isEmpty()) {
+ // check if any duplicates
+ Set<String> names = new HashSet<String>();
+ Set<String> duplicates = criticalOptions.stream().filter(option -> !names.add(option.getName()))
+ .map(option -> option.getName())
+ .collect(Collectors.toSet());
+ if (!duplicates.isEmpty()) {
+ throw new IllegalArgumentException("Duplicate critical option: " + String.join(",", duplicates));
+ }
+ // lexically order by "name"
+ List<OpenSshCertificate.CertificateOption> sortedCriticalOptions = criticalOptions.stream()
+ .sorted(Comparator.comparing(OpenSshCertificate.CertificateOption::getName))
+ .collect(Collectors.toList());
+ this.criticalOptions = sortedCriticalOptions;
+ }
Review Comment:
I missed that part from OpenSSH documentation. And good catch for that bug.
I will apply now same logic for both extensions.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf merged pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
tomaswolf merged PR #222:
URL: https://github.com/apache/mina-sshd/pull/222
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] vukzeka commented on a diff in pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
vukzeka commented on code in PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#discussion_r870194796
##########
sshd-core/src/main/java/org/apache/sshd/certificate/OpenSshCertificateBuilder.java:
##########
@@ -104,7 +110,21 @@ public OpenSshCertificateBuilder principals(Collection<String> principals) {
}
public OpenSshCertificateBuilder criticalOptions(List<OpenSshCertificate.CertificateOption> criticalOptions) {
- this.criticalOptions = criticalOptions;
+ if (criticalOptions != null && !criticalOptions.isEmpty()) {
+ // check if any duplicates
+ Set<String> names = new HashSet<String>();
+ Set<String> duplicates = criticalOptions.stream().filter(option -> !names.add(option.getName()))
+ .map(option -> option.getName())
+ .collect(Collectors.toSet());
+ if (!duplicates.isEmpty()) {
+ throw new IllegalArgumentException("Duplicate critical option: " + String.join(",", duplicates));
+ }
+ // lexically order by "name"
+ List<OpenSshCertificate.CertificateOption> sortedCriticalOptions = criticalOptions.stream()
+ .sorted(Comparator.comparing(OpenSshCertificate.CertificateOption::getName))
+ .collect(Collectors.toList());
+ this.criticalOptions = sortedCriticalOptions;
+ }
Review Comment:
@tomaswolf I was not aware that duplicate and sorting are applied for extensions.
I just checked with ssh-keygen and is possible to add custom extensions and these are not sorted and also duplicates are allowed
for example following
`ssh-keygen -s user_ca -I certN -n user -O extension:test=value -O extension:test=value -O extension:b@b.com=value -O extension:a@a.com=value -O critical:b_critical_option=value -O critical:a_critical_option=value -V +10d user-key.pub`
would generate
`Type: ssh-rsa-cert-v01@openssh.com user certificate
Public key: RSA-CERT SHA256:c6uq2hsYBCROmuFrWz38sIk+0Hlx75l5kOqRo7J1wv8
Signing CA: RSA SHA256:qBnc5EM9d1dlZSfbtnPBh45cAJt5jM7LcLeGDbFOd38 (using rsa-sha2-512)
Key ID: "certN"
Serial: 0
Valid: from 2022-05-11T13:24:00 to 2022-05-21T13:25:15
Principals:
user
Critical Options: (none)
Extensions:
permit-X11-forwarding
permit-agent-forwarding
permit-port-forwarding
permit-pty
permit-user-rc
test UNKNOWN OPTION (len 9)
test UNKNOWN OPTION (len 9)
b@b.com UNKNOWN OPTION (len 9)
a@a.com UNKNOWN OPTION (len 9)`
So it looks to me
- duplicates are checked only for standard extensions
- ordering is applied only for standard extensions
- all custom extensions are added in the order as specified and custom duplicates are allowed
@tomaswolf I can add like that if that sounds correct?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on a diff in pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on code in PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#discussion_r870397695
##########
sshd-core/src/main/java/org/apache/sshd/certificate/OpenSshCertificateBuilder.java:
##########
@@ -104,7 +110,21 @@ public OpenSshCertificateBuilder principals(Collection<String> principals) {
}
public OpenSshCertificateBuilder criticalOptions(List<OpenSshCertificate.CertificateOption> criticalOptions) {
- this.criticalOptions = criticalOptions;
+ if (criticalOptions != null && !criticalOptions.isEmpty()) {
+ // check if any duplicates
+ Set<String> names = new HashSet<String>();
+ Set<String> duplicates = criticalOptions.stream().filter(option -> !names.add(option.getName()))
+ .map(option -> option.getName())
+ .collect(Collectors.toSet());
+ if (!duplicates.isEmpty()) {
+ throw new IllegalArgumentException("Duplicate critical option: " + String.join(",", duplicates));
+ }
+ // lexically order by "name"
+ List<OpenSshCertificate.CertificateOption> sortedCriticalOptions = criticalOptions.stream()
+ .sorted(Comparator.comparing(OpenSshCertificate.CertificateOption::getName))
+ .collect(Collectors.toList());
+ this.criticalOptions = sortedCriticalOptions;
+ }
Review Comment:
I based my comment of the [OpenSSH documentation](https://github.com/openssh/openssh-portable/blob/0086a286ea6bbd11ca9b664ac3bb12b27443d6eb/PROTOCOL.certkeys#L274):
> The encoding and ordering of extensions in this field is identical to that of the critical options, as is the requirement that each name appear only once.
I had not checked what the OpenSSH code actually does. It appears that [auth-options.c](https://github.com/openssh/openssh-portable/blob/0086a286ea6bbd11ca9b664ac3bb12b27443d6eb/auth-options.c#L72) does not enforce _any_ ordering or key uniqueness requirements. [ssh-keygen.c](https://github.com/openssh/openssh-portable/blob/0086a286ea6bbd11ca9b664ac3bb12b27443d6eb/ssh-keygen.c#L1601) appears to only sort, but not check for duplicates, [except](https://github.com/openssh/openssh-portable/blob/0086a286ea6bbd11ca9b664ac3bb12b27443d6eb/ssh-keygen.c#L1964) for `force-command` and `source-address`.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] vukzeka commented on a diff in pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
vukzeka commented on code in PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#discussion_r870067540
##########
sshd-core/src/test/java/org/apache/sshd/certificates/GenerateOpenSSHClientCertificateTest.java:
##########
@@ -154,7 +150,12 @@ public void signCertificate() throws Exception {
.publicKey(clientPublicKey)
.id("user01")
.principals(Collections.singletonList("user01"))
+ .criticalOptions(Arrays.asList(
+ new OpenSshCertificate.CertificateOption("force-command", "/path/to/script.sh"),
+ new OpenSshCertificate.CertificateOption("source-address", "127.0.0.1/32"),
+ new OpenSshCertificate.CertificateOption("verify-required")))
.extensions(Arrays.asList(
+ new OpenSshCertificate.CertificateOption("no-touch-required"),
Review Comment:
hi @tomaswolf
Based on specs I believe no-touch-required should be used only with sk keys.
I added it here for completeness of all available critical options/extensions as in the code **CertificateOptions** are not constants so we do not have enumeration.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] vukzeka commented on a diff in pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
vukzeka commented on code in PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#discussion_r870069420
##########
sshd-core/src/test/java/org/apache/sshd/certificates/GenerateOpenSSHClientCertificateTest.java:
##########
@@ -154,7 +150,12 @@ public void signCertificate() throws Exception {
.publicKey(clientPublicKey)
.id("user01")
.principals(Collections.singletonList("user01"))
+ .criticalOptions(Arrays.asList(
+ new OpenSshCertificate.CertificateOption("force-command", "/path/to/script.sh"),
+ new OpenSshCertificate.CertificateOption("source-address", "127.0.0.1/32"),
+ new OpenSshCertificate.CertificateOption("verify-required")))
Review Comment:
@tomaswolf i didn't add there as would be required to change all existing certs as message is now different meaning signature and message check will not pass.
Probably better would be to add separate test for these. I will see later what would be best and will update
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#issuecomment-1125204883
@tomaswolf
> please downgrade the formatter maven plug-in again. (Revert commit https://github.com/apache/mina-sshd/commit/9f8aa3dbafe3934bad661d36a1d6bf20e80baada.) It doesn't run with Java 1.8
On it... sorry (again)
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] vukzeka commented on pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
vukzeka commented on PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#issuecomment-1125839242
> Looks fine to me. I'll download, undo Lyor's maven formatter change, rebase, run the tests locally, and if successful squash-merge this later.
Thanks @tomaswolf
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] vukzeka commented on a diff in pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
vukzeka commented on code in PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#discussion_r870060409
##########
sshd-common/src/main/java/org/apache/sshd/common/util/buffer/Buffer.java:
##########
@@ -348,29 +348,36 @@ public List<OpenSshCertificate.CertificateOption> getCertificateOptions() {
/**
* According to <A HREF=
- * "https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.certkeys#L222-L262">PROTOCOL.certkeys</A>:
+ * "https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.certkeys#L221-L319">PROTOCOL.certkeys</A>:
*
* Critical Options is a set of bytes that is
*
- * [overall length][name(string)][data(string)]...
+ * [overall length][name(string)][[length of buffer][[length of string][data(string)]]]...
*
- * Where each Critical Option is encoded as a name (string) and data (string)
+ * Where each Certificate Option is encoded as a name (string) and buffer of data (string packed in a buffer)
*
- * Then the entire name + data strings are added as bytes (which will get a length prefix)
+ * Then the entire name (string) + data (buffer) are added as bytes (which will get a length prefix)
*
* @param charset {@link Charset} to use for converting bytes to characters
* @return the parsed result, never {@code null}, but possibly empty
*/
public List<OpenSshCertificate.CertificateOption> getCertificateOptions(Charset charset) {
- // pull out entire Critical Options section
- final ByteArrayBuffer optionBuffer = new ByteArrayBuffer(getBytes());
-
List<OpenSshCertificate.CertificateOption> list = new ArrayList<>();
- while (optionBuffer.available() > 0) {
- String name = optionBuffer.getString(charset);
- String data = GenericUtils.trimToEmpty(optionBuffer.getString(charset));
- list.add(new OpenSshCertificate.CertificateOption(name, data.length() > 0 ? data : null));
+ if (available() > 0) {
+ // pull out entire Certificate Options section
+ final ByteArrayBuffer optionBuffer = new ByteArrayBuffer(getBytes());
Review Comment:
hi @lgoldstein
Thanks for taking time for review.
Actually this was copy-paste from existing code, see https://github.com/apache/mina-sshd/blob/master/sshd-common/src/main/java/org/apache/sshd/common/util/buffer/Buffer.java#L366
In getCertificateOptions I provided it correctly but somehow didn't align these two.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] vukzeka commented on a diff in pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
vukzeka commented on code in PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#discussion_r870194796
##########
sshd-core/src/main/java/org/apache/sshd/certificate/OpenSshCertificateBuilder.java:
##########
@@ -104,7 +110,21 @@ public OpenSshCertificateBuilder principals(Collection<String> principals) {
}
public OpenSshCertificateBuilder criticalOptions(List<OpenSshCertificate.CertificateOption> criticalOptions) {
- this.criticalOptions = criticalOptions;
+ if (criticalOptions != null && !criticalOptions.isEmpty()) {
+ // check if any duplicates
+ Set<String> names = new HashSet<String>();
+ Set<String> duplicates = criticalOptions.stream().filter(option -> !names.add(option.getName()))
+ .map(option -> option.getName())
+ .collect(Collectors.toSet());
+ if (!duplicates.isEmpty()) {
+ throw new IllegalArgumentException("Duplicate critical option: " + String.join(",", duplicates));
+ }
+ // lexically order by "name"
+ List<OpenSshCertificate.CertificateOption> sortedCriticalOptions = criticalOptions.stream()
+ .sorted(Comparator.comparing(OpenSshCertificate.CertificateOption::getName))
+ .collect(Collectors.toList());
+ this.criticalOptions = sortedCriticalOptions;
+ }
Review Comment:
@tomaswolf I was not aware that duplicate and sorting are applied for extensions.
I just checked with ssh-keygen and is possible to add custom extensions and these are not sorted and also duplicates are allowed
for example following
```
ssh-keygen -s user_ca -I certN -n user -O extension:test=value -O extension:test=value -O extension:b@b.com=value -O extension:a@a.com=value -O critical:b_critical_option=value -O critical:a_critical_option=value -V +10d user-key.pub
```
would generate
``` Type: ssh-rsa-cert-v01@openssh.com user certificate
Public key: RSA-CERT SHA256:c6uq2hsYBCROmuFrWz38sIk+0Hlx75l5kOqRo7J1wv8
Signing CA: RSA SHA256:qBnc5EM9d1dlZSfbtnPBh45cAJt5jM7LcLeGDbFOd38 (using rsa-sha2-512)
Key ID: "certN"
Serial: 0
Valid: from 2022-05-11T13:34:00 to 2022-05-21T13:35:41
Principals:
user
Critical Options:
b_critical_option UNKNOWN OPTION (len 9)
a_critical_option UNKNOWN OPTION (len 9)
Extensions:
permit-X11-forwarding
permit-agent-forwarding
permit-port-forwarding
permit-pty
permit-user-rc
test UNKNOWN OPTION (len 9)
test UNKNOWN OPTION (len 9)
b@b.com UNKNOWN OPTION (len 9)
a@a.com UNKNOWN OPTION (len 9)
```
So it looks to me
- duplicates are checked only for standard extensions
- ordering is applied only for standard extensions
- all custom extensions are added in the order as specified and custom duplicates are allowed
@tomaswolf I can add like that if that sounds correct?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] vukzeka commented on a diff in pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
vukzeka commented on code in PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#discussion_r870197910
##########
sshd-common/src/main/java/org/apache/sshd/common/util/buffer/Buffer.java:
##########
@@ -348,29 +348,36 @@ public List<OpenSshCertificate.CertificateOption> getCertificateOptions() {
/**
* According to <A HREF=
- * "https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.certkeys#L222-L262">PROTOCOL.certkeys</A>:
+ * "https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.certkeys#L221-L319">PROTOCOL.certkeys</A>:
*
* Critical Options is a set of bytes that is
*
- * [overall length][name(string)][data(string)]...
+ * [overall length][name(string)][[length of buffer][[length of string][data(string)]]]...
*
- * Where each Critical Option is encoded as a name (string) and data (string)
+ * Where each Certificate Option is encoded as a name (string) and buffer of data (string packed in a buffer)
*
- * Then the entire name + data strings are added as bytes (which will get a length prefix)
+ * Then the entire name (string) + data (buffer) are added as bytes (which will get a length prefix)
*
* @param charset {@link Charset} to use for converting bytes to characters
* @return the parsed result, never {@code null}, but possibly empty
*/
public List<OpenSshCertificate.CertificateOption> getCertificateOptions(Charset charset) {
- // pull out entire Critical Options section
- final ByteArrayBuffer optionBuffer = new ByteArrayBuffer(getBytes());
-
List<OpenSshCertificate.CertificateOption> list = new ArrayList<>();
- while (optionBuffer.available() > 0) {
- String name = optionBuffer.getString(charset);
- String data = GenericUtils.trimToEmpty(optionBuffer.getString(charset));
- list.add(new OpenSshCertificate.CertificateOption(name, data.length() > 0 ? data : null));
+ if (available() > 0) {
+ // pull out entire Certificate Options section
+ final ByteArrayBuffer optionBuffer = new ByteArrayBuffer(getBytes());
+
+ while (optionBuffer.available() > 0) {
+ String name = optionBuffer.getString(charset);
+ String data = null;
+ final ByteArrayBuffer dataBuffer = new ByteArrayBuffer(optionBuffer.getBytes());
Review Comment:
@lgoldstein same as previous will apply all changes together
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] vukzeka commented on a diff in pull request #222: SSHD-1266 - fix for issue with critical option data encoding and smal…
Posted by GitBox <gi...@apache.org>.
vukzeka commented on code in PR #222:
URL: https://github.com/apache/mina-sshd/pull/222#discussion_r870828179
##########
sshd-core/src/test/java/org/apache/sshd/certificates/GenerateOpenSSHClientCertificateTest.java:
##########
@@ -154,7 +150,12 @@ public void signCertificate() throws Exception {
.publicKey(clientPublicKey)
.id("user01")
.principals(Collections.singletonList("user01"))
+ .criticalOptions(Arrays.asList(
+ new OpenSshCertificate.CertificateOption("force-command", "/path/to/script.sh"),
+ new OpenSshCertificate.CertificateOption("source-address", "127.0.0.1/32"),
+ new OpenSshCertificate.CertificateOption("verify-required")))
Review Comment:
ok, just regenerated certs but i have separated them from the old ones as there is other test which is using same and it fails with dummy critical options (source-address, force-command). There is sh script which can be used to easily generate/regenerate if needed
--
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.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org