[
https://issues.apache.org/jira/browse/SSHD-1266?focusedWorklogId=768565&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-768565
]
ASF GitHub Bot logged work on SSHD-1266:
----------------------------------------
Author: ASF GitHub Bot
Created on: 10/May/22 16:04
Start Date: 10/May/22 16:04
Worklog Time Spent: 10m
Work Description: 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...
Issue Time Tracking
-------------------
Worklog Id: (was: 768565)
Time Spent: 20m (was: 10m)
> OpenSSH certificate is not properly encoded when critical options are included
> ------------------------------------------------------------------------------
>
> Key: SSHD-1266
> URL: https://issues.apache.org/jira/browse/SSHD-1266
> Project: MINA SSHD
> Issue Type: Bug
> Affects Versions: 2.8.0
> Reporter: Zeljko Vukovic
> Priority: Minor
> Time Spent: 20m
> Remaining Estimate: 0h
>
> If critical options are included when OpenSSH certificate is created same
> can't be read with OpenSSH.
>
> In oder to reproduce issue we can use existing test
> [https://github.com/apache/mina-sshd/blob/master/sshd-core/src/test/java/org/apache/sshd/certificates/GenerateOpenSSHClientCertificateTest.java#L152]
> and just add critical options (as in the example bellow)
> {code:java}
> final OpenSshCertificate signedCert =
> OpenSshCertificateBuilder.userCertificate()
> .serial(0L)
> .publicKey(clientPublicKey)
> .id("user01")
> .principals(Collections.singletonList("user01"))
>
> .criticalOptions(Arrays.asList(
> new
> OpenSshCertificate.CertificateOption("force-command", "wget url"),
> new
> OpenSshCertificate.CertificateOption("source-address", "127.0.0.1/32")))
>
> .extensions(Arrays.asList(
> new
> OpenSshCertificate.CertificateOption("permit-X11-forwarding"),
> new
> OpenSshCertificate.CertificateOption("permit-agent-forwarding"),
> new
> OpenSshCertificate.CertificateOption("permit-port-forwarding"),
> new
> OpenSshCertificate.CertificateOption("permit-pty"),
> new
> OpenSshCertificate.CertificateOption("permit-user-rc")))
> .sign(caKeypair, signatureAlgorithm); {code}
>
> Once we check such certificate we get following error
> {code:java}
> > ssh-keygen -L -f /path/to/cert.pub
> Type: [email protected] user certificate
> Public key: ECDSA-CERT
> SHA256:0ITcONLKI/H/FNpXZVZMaEYB0STXD4BQNBkSSuDpk5U
> Signing CA: ECDSA SHA256:KPz5LunqQBL9hWJx5eDk11T16anJCn40d/g480PfuKw
> (using ecdsa-sha2-nistp384)
> Key ID: "user01"
> Serial: 0
> Valid: forever
> Principals:
> user01
> Critical Options:
> show_options: buffer error: string is too large {code}
> and similar for the other cert types(RSA, EC, Ed25519).
> I was tracing this issue and it looks like related to this code
> [https://github.com/apache/mina-sshd/blob/master/sshd-common/src/main/java/org/apache/sshd/common/util/buffer/Buffer.java#L840]
> but I was not able to figure out what exactly.
>
> Interesting is that parsing certificate is working as expected
> [https://github.com/apache/mina-sshd/blob/master/sshd-common/src/main/java/org/apache/sshd/common/util/buffer/Buffer.java#L370]
> from code but also even if I create certificate directly with ssh-keygen
> {code:java}
> ssh-keygen -t rsa -b 4096 -f user_ca -C user_ca
> ssh-keygen -f user-key -b 4096 -t rsa
> ssh-keygen -s user_ca -I certN -n user -O source-address="127.0.0.1/32" -O
> force-command="wget url" -V +10d user-key.pub {code}
>
> [[email protected]] / [~twolf] please if any hints what to check(it
> looks to me that there is something wrong with encoding certificate option
> data
> [https://github.com/apache/mina-sshd/blob/master/sshd-common/src/main/java/org/apache/sshd/common/util/buffer/Buffer.java#L838-L845]
> , like these tuples should be written somehow differently) I am more than
> open to support and create PR.
> This is working as expected for extensions as these are all empty(do not have
> data) but once we include critical options which have data than there is
> mentioned failure
> ([https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.certkeys#L221-L268]
> ).
>
>
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
