[ https://issues.apache.org/jira/browse/SSHD-1266?focusedWorklogId=768915&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-768915 ]
ASF GitHub Bot logged work on SSHD-1266: ---------------------------------------- Author: ASF GitHub Bot Created on: 11/May/22 09:10 Start Date: 11/May/22 09:10 Worklog Time Spent: 10m Work Description: 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. Issue Time Tracking ------------------- Worklog Id: (was: 768915) Time Spent: 1h (was: 50m) > 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: 1h > 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: ecdsa-sha2-nistp256-cert-...@openssh.com 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} > > [~alex.sher...@gmail.com] / [~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: dev-unsubscr...@mina.apache.org For additional commands, e-mail: dev-h...@mina.apache.org