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 2020/05/18 16:24:14 UTC
[GitHub] [mina-sshd] jvz opened a new pull request #132: [SSHD-506] Add support for RFC 5647
jvz opened a new pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132
This is a work in progress that does not work yet, and the API changes to Cipher/CipherInformation are still in flux.
I've been testing this by changing `CoreTestSupportUtils` to set up clients and servers with only the AES/GCM cipher enabled. So far, I've had varying problems with getting this to work, and I've been using OpenSSH as a comparison to no further help. For reference, here are the relevant source files:
* [packet.c](https://github.com/openbsd/src/blob/master/usr.bin/ssh/packet.c)
* [cipher.c](https://github.com/openbsd/src/blob/master/usr.bin/ssh/cipher.c) (and maybe a reference to the OpenSSL API to understand all the `EVP_` functions)
* [kex.c](https://github.com/openbsd/src/blob/master/usr.bin/ssh/kex.c) (slightly less relevant)
----------------------------------------------------------------
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:
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 #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-650795345
>> I’m trying to make the changes required for both ciphers to be as similar and minimal as possible. I’ll let you know once this is final again!
Great - this contribution is very important so thank you for the effort...
----------------------------------------------------------------
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:
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] jvz commented on pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-650477660
While working on SSHD-1017, I discovered a couple refactoring improvements to add here.
----------------------------------------------------------------
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:
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] jvz edited a comment on pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz edited a comment on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-637249935
I can't seem to find any official documentation on what the RFC 5647 cipher names are supposed to be unless they meant to literally use `AEAD_AES_128_GCM` and `AEAD_AES_256_GCM` as the cipher names. That seems odd since every other cipher name is in lowercase and uses dashes instead of underscores.
Edit: neither FileZilla nor OpenSSH even reference RFC 5647. Is this the first implementation of the spec in 11 years?
----------------------------------------------------------------
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:
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] jvz commented on pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-637249935
I can't seem to find any official documentation on what the RFC 5647 cipher names are supposed to be unless they meant to literally use `AEAD_AES_128_GCM` and `AEAD_AES_256_GCM` as the cipher names. That seems odd since every other cipher name is in lowercase and uses dashes instead of underscores.
----------------------------------------------------------------
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:
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] jvz commented on a change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r429504620
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sshd.common.cipher;
+
+import java.security.spec.AlgorithmParameterSpec;
+
+import javax.crypto.Cipher;
+import javax.crypto.spec.GCMParameterSpec;
+
+import org.apache.sshd.common.util.buffer.Buffer;
+import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
+
+public class BaseGCMCipher extends BaseAEADCipher {
+
+ public BaseGCMCipher(
+ int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation,
+ int blkSize) {
+ super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
+ }
+
+ @Override
+ protected AlgorithmParameterSpec initializeAlgorithmParameters(byte[] iv) {
+ Buffer buffer = new ByteArrayBuffer(iv);
+ buffer.rpos(Integer.BYTES);
+ long ic = buffer.getLong();
+ // decrement IV as it will be incremented back to initial value on first call to updateWithAAD
+ ic = (ic - 1) & 0x0ffffffffL;
+ buffer.wpos(Integer.BYTES);
+ buffer.putLong(ic);
+ return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, buffer.array());
+ }
+
+ @Override
+ protected AlgorithmParameterSpec getNextAlgorithmParameters() {
+ Cipher cipher = getCipherInstance();
+ Buffer iv = new ByteArrayBuffer(cipher.getIV());
+ iv.rpos(Integer.BYTES);
+ long ic = iv.getLong();
+ ic = (ic + 1) & 0x0ffffffffL;
+ iv.wpos(Integer.BYTES);
+ iv.putLong(ic);
+ return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, iv.array());
+ }
Review comment:
I can't figure out how to get the engine to consider the IV as different, so I still have some clone calls happening, but much less waste than before.
----------------------------------------------------------------
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:
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] jvz commented on pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-665138067
Oh hey, sorry about that. This branch is basically ready at this point as I got stuck trying to get ChaCha20-Poly1305 to work properly. From what I recall, I already updated the code in this branch to match the same style I was hoping to use for that cipher (particularly by making the ChaCha version of `updateAAD` handle the encryption/decryption aspect, too). As that aspect shouldn't change too much, and based on the refactoring culture here, I don't think there's much need to wait any longer on the second feature.
----------------------------------------------------------------
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:
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 change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r426765217
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##########
@@ -1249,6 +1260,20 @@ protected Buffer encode(Buffer buffer) throws IOException {
}
}
+ protected void aeadOutgoingBuffer(Buffer buf, int offset, int len) throws Exception {
+ if (outCipher instanceof BaseGCMCipher) {
Review comment:
I would throw an exception here if this is not a `BaseGCMCipher` since only such ciphers are expected to reach this code
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##########
@@ -1334,9 +1362,19 @@ protected void decode() throws Exception {
assert decoderBuffer.rpos() == Integer.BYTES;
int macSize = (inMac != null) ? inMacSize : 0;
// Check if the packet has been fully received
- if (decoderBuffer.available() >= (decoderLength + macSize)) {
+ if (decoderBuffer.available() >= (decoderLength + macSize + authLen)) {
byte[] data = decoderBuffer.array();
- if (etmMode) {
+ if (authMode) {
+ final BaseGCMCipher cipher = (BaseGCMCipher) inCipher;
Review comment:
No need for `final` - we don't use it in these cases
----------------------------------------------------------------
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:
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 #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-666951872
Committed [dc73e260f](https://github.com/apache/mina-sshd/commit/0ba876638c690cc1441a2cc55266715dc73e260f) with many thanks - you can close the 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.
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] jvz commented on a change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r428112293
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BuiltinCiphers.java
##########
@@ -46,39 +46,48 @@
* @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
*/
public enum BuiltinCiphers implements CipherFactory {
- none(Constants.NONE, 0, 0, "None", 0, "None", 0) {
+ none(Constants.NONE, 0, 0, 0, "None", 0, "None", 0) {
@Override
public Cipher create() {
return new CipherNone();
}
},
- aes128cbc(Constants.AES128_CBC, 16, 16, "AES", 128, "AES/CBC/NoPadding", 16),
- aes128ctr(Constants.AES128_CTR, 16, 16, "AES", 128, "AES/CTR/NoPadding", 16),
- aes192cbc(Constants.AES192_CBC, 16, 24, "AES", 192, "AES/CBC/NoPadding", 16),
- aes192ctr(Constants.AES192_CTR, 16, 24, "AES", 192, "AES/CTR/NoPadding", 16),
- aes256cbc(Constants.AES256_CBC, 16, 32, "AES", 256, "AES/CBC/NoPadding", 16),
- aes256ctr(Constants.AES256_CTR, 16, 32, "AES", 256, "AES/CTR/NoPadding", 16),
- arcfour128(Constants.ARCFOUR128, 8, 16, "ARCFOUR", 128, "RC4", 16) {
+ aes128cbc(Constants.AES128_CBC, 16, 0, 16, "AES", 128, "AES/CBC/NoPadding", 16),
+ aes128ctr(Constants.AES128_CTR, 16, 0, 16, "AES", 128, "AES/CTR/NoPadding", 16),
+ aes128gcm(Constants.AES128_GCM, 12, 16, 16, "AES", 128, "AES/GCM/NoPadding", 16) {
+ @Override
+ public Cipher create() {
+ return new BaseGCMCipher(
+ getIVSize(), getKdfSize(), getAlgorithm(), getKeySize(), getTransformation(), getCipherBlockSize(), getAuthenticationTagSize());
+ }
+ },
+ // TODO: aes256gcm
Review comment:
SSH doesn't seem to support that: https://github.com/openbsd/src/blob/master/usr.bin/ssh/cipher.c
----------------------------------------------------------------
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:
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 change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r439844394
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sshd.common.cipher;
+
+import javax.crypto.Cipher;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.GCMParameterSpec;
+import javax.crypto.spec.SecretKeySpec;
+
+import org.apache.sshd.common.util.buffer.BufferUtils;
+import org.apache.sshd.common.util.security.SecurityUtils;
+
+public class BaseGCMCipher extends BaseCipher {
+
+ private Mode mode;
+ private CounterGCMParameterSpec parameters;
+ private SecretKey secretKey;
Review comment:
Let's make them `protected` in the spirit of open-source and allowing others to extend this class if they want/need to.
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java
##########
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sshd.common.cipher;
+
+import javax.crypto.Cipher;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.GCMParameterSpec;
+import javax.crypto.spec.SecretKeySpec;
+
+import org.apache.sshd.common.util.buffer.BufferUtils;
+import org.apache.sshd.common.util.security.SecurityUtils;
+
+public class BaseGCMCipher extends BaseCipher {
+
+ private Mode mode;
+ private CounterGCMParameterSpec parameters;
+ private SecretKey secretKey;
+
+ public BaseGCMCipher(
+ int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation,
+ int blkSize) {
+ super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
+ }
+
+ @Override
+ protected Cipher createCipherInstance(Mode mode, byte[] key, byte[] iv) throws Exception {
+ this.mode = mode;
+ secretKey = new SecretKeySpec(key, getAlgorithm());
+ parameters = new CounterGCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, iv);
+ parameters.decrementCounter();
+ return SecurityUtils.getCipher(getTransformation());
+ }
+
+ @Override
+ public void update(byte[] input, int inputOffset, int inputLen) throws Exception {
+ updateWithAAD(input, inputOffset, 0, inputLen);
+ }
+
+ @Override
+ public void updateWithAAD(byte[] input, int offset, int aadLen, int inputLen) throws Exception {
+ Cipher cipher = getCipherInstance();
+ parameters.incrementCounter();
+ cipher.init(mode == Mode.Encrypt ? Cipher.ENCRYPT_MODE : Cipher.DECRYPT_MODE, secretKey, parameters);
+ if (aadLen > 0) {
+ cipher.updateAAD(input, offset, aadLen);
+ }
+ int inputOffset = offset + aadLen;
+ if (mode == Mode.Decrypt) {
+ inputLen += getAuthenticationTagSize();
+ }
+ cipher.doFinal(input, inputOffset, inputLen, input, inputOffset);
+ }
+
+ /**
+ * Algorithm parameters for AES/GCM that assumes the IV uses an 8-byte counter field as its most significant bytes.
+ */
+ protected static class CounterGCMParameterSpec extends GCMParameterSpec {
+ private final byte[] iv;
Review comment:
Let's make it`protected` in the spirit of open-source and allowing others to extend this class if they want/need to.
----------------------------------------------------------------
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:
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] jvz commented on a change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r429496552
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sshd.common.cipher;
+
+import java.security.spec.AlgorithmParameterSpec;
+
+import javax.crypto.Cipher;
+import javax.crypto.spec.GCMParameterSpec;
+
+import org.apache.sshd.common.util.buffer.Buffer;
+import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
+
+public class BaseGCMCipher extends BaseAEADCipher {
+
+ public BaseGCMCipher(
+ int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation,
+ int blkSize) {
+ super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
+ }
+
+ @Override
+ protected AlgorithmParameterSpec initializeAlgorithmParameters(byte[] iv) {
+ Buffer buffer = new ByteArrayBuffer(iv);
+ buffer.rpos(Integer.BYTES);
+ long ic = buffer.getLong();
+ // decrement IV as it will be incremented back to initial value on first call to updateWithAAD
+ ic = (ic - 1) & 0x0ffffffffL;
+ buffer.wpos(Integer.BYTES);
+ buffer.putLong(ic);
+ return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, buffer.array());
+ }
+
+ @Override
+ protected AlgorithmParameterSpec getNextAlgorithmParameters() {
+ Cipher cipher = getCipherInstance();
+ Buffer iv = new ByteArrayBuffer(cipher.getIV());
+ iv.rpos(Integer.BYTES);
+ long ic = iv.getLong();
+ ic = (ic + 1) & 0x0ffffffffL;
+ iv.wpos(Integer.BYTES);
+ iv.putLong(ic);
+ return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, iv.array());
+ }
Review comment:
I came up with a better idea: extend the class and don't clone the IV. Less safe, but doesn't continually create garbage.
----------------------------------------------------------------
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:
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] jvz closed pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz closed pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132
----------------------------------------------------------------
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:
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] jvz commented on pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-667143520
Thanks!
----------------------------------------------------------------
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:
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 change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r429326530
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sshd.common.cipher;
+
+import java.security.spec.AlgorithmParameterSpec;
+
+import javax.crypto.Cipher;
+import javax.crypto.spec.GCMParameterSpec;
+
+import org.apache.sshd.common.util.buffer.Buffer;
+import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
+
+public class BaseGCMCipher extends BaseAEADCipher {
+
+ public BaseGCMCipher(
+ int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation,
+ int blkSize) {
+ super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
+ }
+
+ @Override
+ protected AlgorithmParameterSpec initializeAlgorithmParameters(byte[] iv) {
+ Buffer buffer = new ByteArrayBuffer(iv);
+ buffer.rpos(Integer.BYTES);
+ long ic = buffer.getLong();
+ // decrement IV as it will be incremented back to initial value on first call to updateWithAAD
+ ic = (ic - 1) & 0x0ffffffffL;
+ buffer.wpos(Integer.BYTES);
+ buffer.putLong(ic);
+ return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, buffer.array());
+ }
+
+ @Override
+ protected AlgorithmParameterSpec getNextAlgorithmParameters() {
+ Cipher cipher = getCipherInstance();
+ Buffer iv = new ByteArrayBuffer(cipher.getIV());
+ iv.rpos(Integer.BYTES);
+ long ic = iv.getLong();
+ ic = (ic + 1) & 0x0ffffffffL;
+ iv.wpos(Integer.BYTES);
+ iv.putLong(ic);
+ return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, iv.array());
+ }
Review comment:
Pretty expensive in memory - every time it is called (which is for every packet) a new `byte[]` (`cipher.getIv()` returns a **clone**), new `Buffer` and new `GCMParameterSpec` instance is created. Can we do something about it ?
Moreover, this code is exactly the same as `initializeAlgorithmParameters` except it does `+1` instead of `-1` - perhaps we could use an internal (protected) `updateAlgorithmParameters(byte[] iv, long delta)` and call it once with `+1` and once with `-1`
----------------------------------------------------------------
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:
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] jvz commented on pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-632271382
Yes, these should be added as defaults once they're working. Based on how OpenSSH is distributed nowadays, these should the highest priority ciphers. I've seen ChaCha20 taking precedence as well, though that's not supported here yet (plus requires either Java 11 or Bouncycastle or some other library).
----------------------------------------------------------------
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:
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] jvz commented on pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-632976351
The tests all pass on my machine. I'm not sure what's going on with the failing one on here.
----------------------------------------------------------------
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:
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] jvz commented on a change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r429500880
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sshd.common.cipher;
+
+import java.security.spec.AlgorithmParameterSpec;
+
+import javax.crypto.Cipher;
+import javax.crypto.spec.GCMParameterSpec;
+
+import org.apache.sshd.common.util.buffer.Buffer;
+import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
+
+public class BaseGCMCipher extends BaseAEADCipher {
+
+ public BaseGCMCipher(
+ int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation,
+ int blkSize) {
+ super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
+ }
+
+ @Override
+ protected AlgorithmParameterSpec initializeAlgorithmParameters(byte[] iv) {
+ Buffer buffer = new ByteArrayBuffer(iv);
+ buffer.rpos(Integer.BYTES);
+ long ic = buffer.getLong();
+ // decrement IV as it will be incremented back to initial value on first call to updateWithAAD
+ ic = (ic - 1) & 0x0ffffffffL;
+ buffer.wpos(Integer.BYTES);
+ buffer.putLong(ic);
+ return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, buffer.array());
+ }
+
+ @Override
+ protected AlgorithmParameterSpec getNextAlgorithmParameters() {
+ Cipher cipher = getCipherInstance();
+ Buffer iv = new ByteArrayBuffer(cipher.getIV());
+ iv.rpos(Integer.BYTES);
+ long ic = iv.getLong();
+ ic = (ic + 1) & 0x0ffffffffL;
+ iv.wpos(Integer.BYTES);
+ iv.putLong(ic);
+ return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, iv.array());
+ }
Review comment:
Wow, I optimized it so much, both AES/GCM engines can't tell that the IV changes between invocations and throw an exception. Nice.
----------------------------------------------------------------
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:
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] jvz commented on pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-637222208
Also tested with FileZilla so far. It even defaulted to selecting aes256-gcm! Seems to work.
----------------------------------------------------------------
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:
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 change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r428103603
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sshd.common.cipher;
+
+import org.apache.sshd.common.util.buffer.Buffer;
+import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
+import org.apache.sshd.common.util.security.SecurityUtils;
+
+import javax.crypto.Cipher;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.GCMParameterSpec;
+import javax.crypto.spec.SecretKeySpec;
+
+public class BaseGCMCipher extends BaseCipher {
+
+ private Mode mode;
+ private SecretKey secretKey;
+
+ public BaseGCMCipher(
+ final int ivsize, final int kdfSize, final String algorithm, final int keySize, final String transformation,
+ final int blkSize, final int authSize) {
+ super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
+ }
+
+ @Override
+ protected byte[] initializeKeyData(final Mode mode, final byte[] key, final int reqLen) {
+ byte[] keyData = super.initializeKeyData(mode, key, reqLen);
+ secretKey = new SecretKeySpec(keyData, getAlgorithm());
+ return keyData;
+ }
+
+ @Override
+ protected Cipher createCipherInstance(final Mode mode, final byte[] key, final byte[] iv) throws Exception {
Review comment:
Please remove `final` modifier from parameters
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BuiltinCiphers.java
##########
@@ -46,39 +46,48 @@
* @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
*/
public enum BuiltinCiphers implements CipherFactory {
- none(Constants.NONE, 0, 0, "None", 0, "None", 0) {
+ none(Constants.NONE, 0, 0, 0, "None", 0, "None", 0) {
@Override
public Cipher create() {
return new CipherNone();
}
},
- aes128cbc(Constants.AES128_CBC, 16, 16, "AES", 128, "AES/CBC/NoPadding", 16),
- aes128ctr(Constants.AES128_CTR, 16, 16, "AES", 128, "AES/CTR/NoPadding", 16),
- aes192cbc(Constants.AES192_CBC, 16, 24, "AES", 192, "AES/CBC/NoPadding", 16),
- aes192ctr(Constants.AES192_CTR, 16, 24, "AES", 192, "AES/CTR/NoPadding", 16),
- aes256cbc(Constants.AES256_CBC, 16, 32, "AES", 256, "AES/CBC/NoPadding", 16),
- aes256ctr(Constants.AES256_CTR, 16, 32, "AES", 256, "AES/CTR/NoPadding", 16),
- arcfour128(Constants.ARCFOUR128, 8, 16, "ARCFOUR", 128, "RC4", 16) {
+ aes128cbc(Constants.AES128_CBC, 16, 0, 16, "AES", 128, "AES/CBC/NoPadding", 16),
+ aes128ctr(Constants.AES128_CTR, 16, 0, 16, "AES", 128, "AES/CTR/NoPadding", 16),
+ aes128gcm(Constants.AES128_GCM, 12, 16, 16, "AES", 128, "AES/GCM/NoPadding", 16) {
+ @Override
+ public Cipher create() {
+ return new BaseGCMCipher(
+ getIVSize(), getKdfSize(), getAlgorithm(), getKeySize(), getTransformation(), getCipherBlockSize(), getAuthenticationTagSize());
+ }
+ },
+ // TODO: aes256gcm
Review comment:
Also AES192 GCM
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sshd.common.cipher;
+
+import org.apache.sshd.common.util.buffer.Buffer;
+import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
+import org.apache.sshd.common.util.security.SecurityUtils;
+
+import javax.crypto.Cipher;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.GCMParameterSpec;
+import javax.crypto.spec.SecretKeySpec;
+
+public class BaseGCMCipher extends BaseCipher {
+
+ private Mode mode;
+ private SecretKey secretKey;
+
+ public BaseGCMCipher(
+ final int ivsize, final int kdfSize, final String algorithm, final int keySize, final String transformation,
+ final int blkSize, final int authSize) {
+ super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
+ }
+
+ @Override
+ protected byte[] initializeKeyData(final Mode mode, final byte[] key, final int reqLen) {
Review comment:
Please remove `final` modifier from parameters
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sshd.common.cipher;
+
+import org.apache.sshd.common.util.buffer.Buffer;
+import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
+import org.apache.sshd.common.util.security.SecurityUtils;
+
+import javax.crypto.Cipher;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.GCMParameterSpec;
+import javax.crypto.spec.SecretKeySpec;
+
+public class BaseGCMCipher extends BaseCipher {
+
+ private Mode mode;
+ private SecretKey secretKey;
+
+ public BaseGCMCipher(
+ final int ivsize, final int kdfSize, final String algorithm, final int keySize, final String transformation,
+ final int blkSize, final int authSize) {
+ super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
+ }
+
+ @Override
+ protected byte[] initializeKeyData(final Mode mode, final byte[] key, final int reqLen) {
+ byte[] keyData = super.initializeKeyData(mode, key, reqLen);
+ secretKey = new SecretKeySpec(keyData, getAlgorithm());
+ return keyData;
+ }
+
+ @Override
+ protected Cipher createCipherInstance(final Mode mode, final byte[] key, final byte[] iv) throws Exception {
+ this.mode = mode;
+ Cipher instance = SecurityUtils.getCipher(getTransformation());
+ instance.init(
+ Mode.Encrypt.equals(this.mode) ? Cipher.ENCRYPT_MODE : Cipher.DECRYPT_MODE,
+ secretKey,
+ new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, iv));
+ return instance;
+ }
+
+ public void incrementIV() throws Exception {
+ Cipher cipher = getCipherInstance();
+ Buffer iv = new ByteArrayBuffer(cipher.getIV());
+ iv.rpos(Integer.BYTES);
+ long ic = iv.getLong();
+ ic = (ic + 1) & 0x0ffffffffL;
+ iv.wpos(Integer.BYTES);
+ iv.putLong(ic);
+ cipher.init(
+ mode == Mode.Encrypt ? Cipher.ENCRYPT_MODE : Cipher.DECRYPT_MODE,
+ secretKey,
+ new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, iv.array()));
+ }
+
+ public void updateAAD(final byte[] additionalAuthenticatedData, final int aadOffset, final int aadLength) {
Review comment:
Please remove `final` modifier from parameters
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BuiltinCiphers.java
##########
@@ -87,10 +96,11 @@ public Cipher create() {
private final boolean supported;
BuiltinCiphers(
- String factoryName, int ivsize, int kdfSize,
Review comment:
Please remove `final` modifier from `authSize`
----------------------------------------------------------------
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:
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] jvz commented on pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-632408690
So it seems I've created an incompatible implementation with OpenSSH at the moment, but it talks to itself. Still debugging this before this can move anywhere.
----------------------------------------------------------------
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:
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] jvz commented on pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-637901360
Found one OSS implementation of the RFC version (disabled by default) in Erlang OTP. Seems like the ambiguity about requiring a mac with an AEAD cipher caused OpenSSH to create their variant in the first place. Docs: https://manpages.ubuntu.com/manpages/bionic/man7/ssh.7.html
More notes here about the official ciphers: https://www.iana.org/assignments/ssh-parameters/ssh-parameters.xhtml
Even if we were to take the spec literally, we could create another instance of `Cipher.getInstance("AES/GCM/NoPadding")` and then only use the methods `updateAAD()` and `doFinal()` to use it in GMAC mode. The problem there is that the mac still wouldn't necessarily be used since it's already built in to an AEAD operation.
I'll note this ambiguity is avoided in the OpenSSH ChaCha protocol: https://cvsweb.openbsd.org/src/usr.bin/ssh/PROTOCOL.chacha20poly1305?annotate=HEAD
----------------------------------------------------------------
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:
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] jvz commented on a change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r427733788
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##########
@@ -1334,9 +1362,19 @@ protected void decode() throws Exception {
assert decoderBuffer.rpos() == Integer.BYTES;
int macSize = (inMac != null) ? inMacSize : 0;
// Check if the packet has been fully received
- if (decoderBuffer.available() >= (decoderLength + macSize)) {
+ if (decoderBuffer.available() >= (decoderLength + macSize + authLen)) {
byte[] data = decoderBuffer.array();
- if (etmMode) {
+ if (authMode) {
+ final BaseGCMCipher cipher = (BaseGCMCipher) inCipher;
+ cipher.incrementIV();
+ // RFC 5647: packet length encoded in additional data and unencrypted
+ int off = decoderBuffer.rpos();
+ cipher.updateAAD(data, off - Integer.BYTES, Integer.BYTES);
+ cipher.update(data, off, decoderLength);
+ cipher.doFinal(data, Integer.BYTES + decoderLength);
Review comment:
Finally, I figured out one of the problems. This use of the Cipher class is incorrect. When verifying an AT, that should be processed in `update` rather than `doFinal`.
----------------------------------------------------------------
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:
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] jvz commented on pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-637216880
I merged the latest changes from master and did some basic smoketesting using the following:
Server: OpenSSH_8.1p1, LibreSSL 2.7.3
Command: `./ssh.sh -c 'aes128-gcm@openssh.com' localhost`
I'll do some more testing in the opposite direction as well as some cleanups.
----------------------------------------------------------------
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:
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] jvz commented on pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-643795423
> I will do so some time soon - does this mean that I can merge this patch if I am satisfied with it ?
Yes, I think so. I'm a little iffy on the `BaseGCMCipher` name for the class as I'm imagining a different use of AES/GCM along the lines of how OpenSSH implements ChaCha20-Poly1305 which would behave differently (the additional authenticated data would be encrypted but not authenticated using another AES key using AES/CTR like how in the packet length is encrypted). Otherwise ready to merge.
----------------------------------------------------------------
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:
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] jvz commented on a change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r439770856
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sshd.common.cipher;
+
+import java.security.spec.AlgorithmParameterSpec;
+
+import javax.crypto.Cipher;
+import javax.crypto.spec.GCMParameterSpec;
+
+import org.apache.sshd.common.util.buffer.Buffer;
+import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
+
+public class BaseGCMCipher extends BaseAEADCipher {
+
+ public BaseGCMCipher(
+ int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation,
+ int blkSize) {
+ super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
+ }
+
+ @Override
+ protected AlgorithmParameterSpec initializeAlgorithmParameters(byte[] iv) {
+ Buffer buffer = new ByteArrayBuffer(iv);
+ buffer.rpos(Integer.BYTES);
+ long ic = buffer.getLong();
+ // decrement IV as it will be incremented back to initial value on first call to updateWithAAD
+ ic = (ic - 1) & 0x0ffffffffL;
+ buffer.wpos(Integer.BYTES);
+ buffer.putLong(ic);
+ return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, buffer.array());
+ }
+
+ @Override
+ protected AlgorithmParameterSpec getNextAlgorithmParameters() {
+ Cipher cipher = getCipherInstance();
+ Buffer iv = new ByteArrayBuffer(cipher.getIV());
+ iv.rpos(Integer.BYTES);
+ long ic = iv.getLong();
+ ic = (ic + 1) & 0x0ffffffffL;
+ iv.wpos(Integer.BYTES);
+ iv.putLong(ic);
+ return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, iv.array());
+ }
Review comment:
I looked into the internal implementation of the cipher engine, and it makes its own clone regardless of what we do, so this seems to be about as optimal as I can get it without digging into Bouncycastle internals or something similar.
----------------------------------------------------------------
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:
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] jvz edited a comment on pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz edited a comment on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-637901360
Found one OSS implementation of the RFC version (disabled by default) in Erlang OTP. Seems like the ambiguity about requiring a mac with an AEAD cipher caused OpenSSH to create their variant in the first place. Docs: https://manpages.ubuntu.com/manpages/bionic/man7/ssh.7.html
More notes here about the official ciphers: https://www.iana.org/assignments/ssh-parameters/ssh-parameters.xhtml
in RFC 4253, we have the following:
> The chosen encryption algorithm to each direction MUST be the first algorithm on the client's name-list that is also on the server's name-list.
and
> The chosen MAC algorithm MUST be the first algorithm on the client's name-list that is also on the server's name-list.
Then we go back to RFC 5647 which says:
> If AES-GCM is selected as the encryption algorithm for a given tunnel, AES-GCM MUST also be selected as the Message Authentication Code (MAC) algorithm. Conversely, if AES-GCM is selected as the MAC algorithm, it MUST also be selected as the encryption algorithm.
I'll note this ambiguity is avoided in the OpenSSH ChaCha protocol: https://cvsweb.openbsd.org/src/usr.bin/ssh/PROTOCOL.chacha20poly1305?annotate=HEAD
Edit: I found another project: https://commoncriteria.github.io/pp/ssh/ssh-release.html
----------------------------------------------------------------
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:
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] jvz commented on a change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r428968052
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseAEADCipher.java
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sshd.common.cipher;
+
+import org.apache.sshd.common.util.security.SecurityUtils;
+
+import javax.crypto.Cipher;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.SecretKeySpec;
+import java.security.InvalidAlgorithmParameterException;
+import java.security.InvalidKeyException;
+import java.security.spec.AlgorithmParameterSpec;
+
+public abstract class BaseAEADCipher extends BaseCipher {
+
+ private Mode mode;
+ private SecretKey secretKey;
+ private AlgorithmParameterSpec params;
+ private boolean aadWritten;
+
+ public BaseAEADCipher(
+ int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation, int blkSize) {
+ super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
+ }
+
+ @Override
+ protected byte[] initializeIVData(final Mode mode, final byte[] iv, final int reqLen) {
+ byte[] data = super.initializeIVData(mode, iv, reqLen);
+ params = initializeAlgorithmParameters(data);
+ return data;
+ }
+
+ @Override
+ protected byte[] initializeKeyData(final Mode mode, final byte[] key, final int reqLen) {
+ byte[] keyData = super.initializeKeyData(mode, key, reqLen);
+ secretKey = new SecretKeySpec(keyData, getAlgorithm());
+ return keyData;
+ }
+
+ @Override
+ protected Cipher createCipherInstance(final Mode mode, final byte[] key, final byte[] iv) throws Exception {
+ this.mode = mode;
+ Cipher instance = SecurityUtils.getCipher(getTransformation());
+ init(instance);
+ return instance;
+ }
+
+ private void init(Cipher cipher) throws InvalidKeyException, InvalidAlgorithmParameterException {
Review comment:
Seems I was a little off. The default ciphers in OpenSSH right now are `chacha20-poly1305@openssh.com,aes128-ctr,aes192-ctr,aes256-ctr,aes128-gcm@openssh.com,aes256-gcm@openssh.com`
----------------------------------------------------------------
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:
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] jvz commented on a change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r428836627
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseAEADCipher.java
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sshd.common.cipher;
+
+import org.apache.sshd.common.util.security.SecurityUtils;
+
+import javax.crypto.Cipher;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.SecretKeySpec;
+import java.security.InvalidAlgorithmParameterException;
+import java.security.InvalidKeyException;
+import java.security.spec.AlgorithmParameterSpec;
+
+public abstract class BaseAEADCipher extends BaseCipher {
+
+ private Mode mode;
+ private SecretKey secretKey;
+ private AlgorithmParameterSpec params;
+ private boolean aadWritten;
+
+ public BaseAEADCipher(
+ int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation, int blkSize) {
+ super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
+ }
+
+ @Override
+ protected byte[] initializeIVData(final Mode mode, final byte[] iv, final int reqLen) {
+ byte[] data = super.initializeIVData(mode, iv, reqLen);
+ params = initializeAlgorithmParameters(data);
+ return data;
+ }
+
+ @Override
+ protected byte[] initializeKeyData(final Mode mode, final byte[] key, final int reqLen) {
+ byte[] keyData = super.initializeKeyData(mode, key, reqLen);
+ secretKey = new SecretKeySpec(keyData, getAlgorithm());
+ return keyData;
+ }
+
+ @Override
+ protected Cipher createCipherInstance(final Mode mode, final byte[] key, final byte[] iv) throws Exception {
+ this.mode = mode;
+ Cipher instance = SecurityUtils.getCipher(getTransformation());
+ init(instance);
+ return instance;
+ }
+
+ private void init(Cipher cipher) throws InvalidKeyException, InvalidAlgorithmParameterException {
Review comment:
Probably makes sense. I was trying to refactor this class a little so that supporting ChaCha20-Poly1305 later would be simpler since it's another AEAD algorithm, though it uses a slightly different key derivation algorithm in order to encrypt the packet length as well (wonder if an update or extension to AES/GCM can be made to do the same eventually).
----------------------------------------------------------------
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:
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 #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-633253726
>> The tests all pass on my machine. I'm not sure what's going on with the failing one on here.
That's OK - if you feel the code is ready as far as you are concerned, let me know. Once I have examined it and successfully ran a full `mvn clean install` on my machine I am willing to merge it. Our Travis builds are not yet 100% stable so if they fail but the tests succeed on my machine I usually consider the code safe to merge.
That being said, please run some tests with OpenSSH clients/servers that support these ciphers as well as some popular applications such as _Putty, WinSCP, FileZilla_
----------------------------------------------------------------
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:
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 change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r429651878
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java
##########
@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sshd.common.cipher;
+
+import javax.crypto.Cipher;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.GCMParameterSpec;
+import javax.crypto.spec.SecretKeySpec;
+
+import org.apache.sshd.common.util.security.SecurityUtils;
+
+public class BaseGCMCipher extends BaseCipher {
+
+ private Mode mode;
+ private CounterGCMParameterSpec parameters;
+ private SecretKey secretKey;
+
+ public BaseGCMCipher(
+ int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation,
+ int blkSize) {
+ super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
+ }
+
+ @Override
+ protected Cipher createCipherInstance(Mode mode, byte[] key, byte[] iv) throws Exception {
+ this.mode = mode;
+ secretKey = new SecretKeySpec(key, getAlgorithm());
+ parameters = new CounterGCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, iv);
+ parameters.decrementCounter();
+ return SecurityUtils.getCipher(getTransformation());
+ }
+
+ @Override
+ public void update(byte[] input, int inputOffset, int inputLen) throws Exception {
+ Cipher cipher = getCipherInstance();
+ parameters.incrementCounter();
+ cipher.init(mode == Mode.Encrypt ? Cipher.ENCRYPT_MODE : Cipher.DECRYPT_MODE, secretKey, parameters);
+ cipher.doFinal(input, inputOffset, inputLen, input, inputOffset);
+ }
+
+ @Override
+ public void updateWithAAD(byte[] input, int aadOffset, int aadLen, int inputOffset, int inputLen) throws Exception {
+ Cipher cipher = getCipherInstance();
+ parameters.incrementCounter();
+ cipher.init(mode == Mode.Encrypt ? Cipher.ENCRYPT_MODE : Cipher.DECRYPT_MODE, secretKey, parameters);
+ cipher.updateAAD(input, aadOffset, aadLen);
+ cipher.doFinal(input, inputOffset, inputLen, input, inputOffset);
+ }
+
+ /**
+ * Algorithm parameters for AES/GCM that assumes the IV uses an 8-byte counter field as its most significant bytes.
+ */
+ protected static class CounterGCMParameterSpec extends GCMParameterSpec {
+ private final byte[] iv;
+
+ protected CounterGCMParameterSpec(int tLen, byte[] src) {
+ super(tLen, src);
+ iv = src;
+ }
+
+ protected void incrementCounter() {
+ for (int i = iv.length - 1; i >= iv.length - Long.BYTES; i--) {
+ iv[i]++;
+ if (iv[i] != 0) {
+ break; // no carry
+ }
+ }
+ }
Review comment:
Looks pretty dangerous 2's complement arithmetic - I would rather use a `decodeLong/encodeLong` method...
----------------------------------------------------------------
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:
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] jvz commented on pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-650583844
Once I’ve figured out the ChaCha part, I think this PR will be ready to go.
I’ll add that cipher in a separate PR, but I’m trying to make the changes
required for both ciphers to be as similar and minimal as possible. I’ll
let you know once this is final again!
On Sat, Jun 27, 2020 at 01:12 Lyor Goldstein <no...@github.com>
wrote:
> Seems like it is still work in progress - I will hold off testing it and
> merging into master branch for now
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/mina-sshd/pull/132#issuecomment-650503254>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAGBF23JQGPESOSGRSUTI6DRYWEV5ANCNFSM4NEHJDRQ>
> .
>
--
Matt Sicker <bo...@gmail.com>
----------------------------------------------------------------
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:
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] jvz commented on a change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r428111356
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sshd.common.cipher;
+
+import org.apache.sshd.common.util.buffer.Buffer;
+import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
+import org.apache.sshd.common.util.security.SecurityUtils;
+
+import javax.crypto.Cipher;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.GCMParameterSpec;
+import javax.crypto.spec.SecretKeySpec;
+
+public class BaseGCMCipher extends BaseCipher {
+
+ private Mode mode;
+ private SecretKey secretKey;
+
+ public BaseGCMCipher(
+ final int ivsize, final int kdfSize, final String algorithm, final int keySize, final String transformation,
+ final int blkSize, final int authSize) {
+ super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
+ }
+
+ @Override
+ protected byte[] initializeKeyData(final Mode mode, final byte[] key, final int reqLen) {
Review comment:
I updated my code formatting settings. I had initially imported the bundled eclipse xml file. Will have a fix in my next commit.
----------------------------------------------------------------
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:
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 change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r429037330
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BuiltinCiphers.java
##########
@@ -46,39 +46,48 @@
* @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
*/
public enum BuiltinCiphers implements CipherFactory {
- none(Constants.NONE, 0, 0, "None", 0, "None", 0) {
+ none(Constants.NONE, 0, 0, 0, "None", 0, "None", 0) {
@Override
public Cipher create() {
return new CipherNone();
}
},
- aes128cbc(Constants.AES128_CBC, 16, 16, "AES", 128, "AES/CBC/NoPadding", 16),
- aes128ctr(Constants.AES128_CTR, 16, 16, "AES", 128, "AES/CTR/NoPadding", 16),
- aes192cbc(Constants.AES192_CBC, 16, 24, "AES", 192, "AES/CBC/NoPadding", 16),
- aes192ctr(Constants.AES192_CTR, 16, 24, "AES", 192, "AES/CTR/NoPadding", 16),
- aes256cbc(Constants.AES256_CBC, 16, 32, "AES", 256, "AES/CBC/NoPadding", 16),
- aes256ctr(Constants.AES256_CTR, 16, 32, "AES", 256, "AES/CTR/NoPadding", 16),
- arcfour128(Constants.ARCFOUR128, 8, 16, "ARCFOUR", 128, "RC4", 16) {
+ aes128cbc(Constants.AES128_CBC, 16, 0, 16, "AES", 128, "AES/CBC/NoPadding", 16),
+ aes128ctr(Constants.AES128_CTR, 16, 0, 16, "AES", 128, "AES/CTR/NoPadding", 16),
+ aes128gcm(Constants.AES128_GCM, 12, 16, 16, "AES", 128, "AES/GCM/NoPadding", 16) {
+ @Override
+ public Cipher create() {
+ return new BaseGCMCipher(
+ getIVSize(), getKdfSize(), getAlgorithm(), getKeySize(), getTransformation(), getCipherBlockSize(), getAuthenticationTagSize());
+ }
+ },
+ // TODO: aes256gcm
Review comment:
I checked the documentation as well and you are right.
----------------------------------------------------------------
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:
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 #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-643786909
>> Feel free to test in any other platforms you're concerned with, but I feel fairly confident with the patch.
I will do so some time soon - does this mean that I can merge this patch if I am satisfied with it ?
----------------------------------------------------------------
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:
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] jvz commented on pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-647042561
Tested on an RPi4 (arm64), works fine.
----------------------------------------------------------------
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:
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] jvz commented on a change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r429363158
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sshd.common.cipher;
+
+import java.security.spec.AlgorithmParameterSpec;
+
+import javax.crypto.Cipher;
+import javax.crypto.spec.GCMParameterSpec;
+
+import org.apache.sshd.common.util.buffer.Buffer;
+import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
+
+public class BaseGCMCipher extends BaseAEADCipher {
+
+ public BaseGCMCipher(
+ int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation,
+ int blkSize) {
+ super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
+ }
+
+ @Override
+ protected AlgorithmParameterSpec initializeAlgorithmParameters(byte[] iv) {
+ Buffer buffer = new ByteArrayBuffer(iv);
+ buffer.rpos(Integer.BYTES);
+ long ic = buffer.getLong();
+ // decrement IV as it will be incremented back to initial value on first call to updateWithAAD
+ ic = (ic - 1) & 0x0ffffffffL;
+ buffer.wpos(Integer.BYTES);
+ buffer.putLong(ic);
+ return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, buffer.array());
+ }
+
+ @Override
+ protected AlgorithmParameterSpec getNextAlgorithmParameters() {
+ Cipher cipher = getCipherInstance();
+ Buffer iv = new ByteArrayBuffer(cipher.getIV());
+ iv.rpos(Integer.BYTES);
+ long ic = iv.getLong();
+ ic = (ic + 1) & 0x0ffffffffL;
+ iv.wpos(Integer.BYTES);
+ iv.putLong(ic);
+ return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, iv.array());
+ }
Review comment:
Ooh, nice find. Yes, we just need to update the 8-byte section of the buffer. The update algorithm parameters API idea sounds good to me; it's more in line with how OpenSSH is structured, too.
----------------------------------------------------------------
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:
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 change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r429383126
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sshd.common.cipher;
+
+import java.security.spec.AlgorithmParameterSpec;
+
+import javax.crypto.Cipher;
+import javax.crypto.spec.GCMParameterSpec;
+
+import org.apache.sshd.common.util.buffer.Buffer;
+import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
+
+public class BaseGCMCipher extends BaseAEADCipher {
+
+ public BaseGCMCipher(
+ int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation,
+ int blkSize) {
+ super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
+ }
+
+ @Override
+ protected AlgorithmParameterSpec initializeAlgorithmParameters(byte[] iv) {
+ Buffer buffer = new ByteArrayBuffer(iv);
+ buffer.rpos(Integer.BYTES);
+ long ic = buffer.getLong();
+ // decrement IV as it will be incremented back to initial value on first call to updateWithAAD
+ ic = (ic - 1) & 0x0ffffffffL;
+ buffer.wpos(Integer.BYTES);
+ buffer.putLong(ic);
+ return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, buffer.array());
+ }
+
+ @Override
+ protected AlgorithmParameterSpec getNextAlgorithmParameters() {
+ Cipher cipher = getCipherInstance();
+ Buffer iv = new ByteArrayBuffer(cipher.getIV());
+ iv.rpos(Integer.BYTES);
+ long ic = iv.getLong();
+ ic = (ic + 1) & 0x0ffffffffL;
+ iv.wpos(Integer.BYTES);
+ iv.putLong(ic);
+ return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, iv.array());
+ }
Review comment:
Regarding the `GCMParameterSpec` based on [this link](https://alvinalexander.com/java/jwarehouse/openjdk-8/jdk/src/share/classes/javax/crypto/spec/GCMParameterSpec.java.shtml) we might be able to execute an ugly hack via reflection API and manipulate the internal IV data directly instead of creating a new instance every time... will need to think about it some more - but only **after** this code is working.
----------------------------------------------------------------
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:
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 change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r429330029
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sshd.common.cipher;
+
+import java.security.spec.AlgorithmParameterSpec;
+
+import javax.crypto.Cipher;
+import javax.crypto.spec.GCMParameterSpec;
+
+import org.apache.sshd.common.util.buffer.Buffer;
+import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
+
+public class BaseGCMCipher extends BaseAEADCipher {
+
+ public BaseGCMCipher(
+ int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation,
+ int blkSize) {
+ super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
+ }
+
+ @Override
+ protected AlgorithmParameterSpec initializeAlgorithmParameters(byte[] iv) {
+ Buffer buffer = new ByteArrayBuffer(iv);
+ buffer.rpos(Integer.BYTES);
+ long ic = buffer.getLong();
+ // decrement IV as it will be incremented back to initial value on first call to updateWithAAD
+ ic = (ic - 1) & 0x0ffffffffL;
+ buffer.wpos(Integer.BYTES);
+ buffer.putLong(ic);
+ return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, buffer.array());
+ }
+
+ @Override
+ protected AlgorithmParameterSpec getNextAlgorithmParameters() {
+ Cipher cipher = getCipherInstance();
+ Buffer iv = new ByteArrayBuffer(cipher.getIV());
+ iv.rpos(Integer.BYTES);
+ long ic = iv.getLong();
+ ic = (ic + 1) & 0x0ffffffffL;
+ iv.wpos(Integer.BYTES);
+ iv.putLong(ic);
+ return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, iv.array());
+ }
Review comment:
I believe we can avoid creating the `Buffer` since all the code does is replace an 8-byte section of the IV at offset 4:
```java
byte[] iv = cipher.getIV();
// Need to write decodeLong but it should be similar to decodeInt in the same class
// can use the code of Buffer#getLong as base
long ic = KeyEntryResolver.decodeLong(iv, Integer.BYTES, iv.length - Integer.BYTES);
ic = (ic + delta) & 0x0ffffffffL;
// Need to write it - can use the code of Buffer#putLong as base
KeyEntryResolver.encodeLong(iv, ic, Integer.BYTES, iv.length - Integer.BYTES);
```
----------------------------------------------------------------
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:
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] jvz commented on pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-633254174
I still need to do some manual integration testing. Thanks for the
suggestions since OpenSSH was the only thing I remembered exists.
This may be on hold for a week or so as I’m on vacation this week.
On Sun, May 24, 2020 at 11:11 Lyor Goldstein <no...@github.com>
wrote:
> The tests all pass on my machine. I'm not sure what's going on with the
> failing one on here.
>
> That's OK - if you feel the code is ready as far as you are concerned, let
> me know. Once I have examined it and successfully ran a full mvn clean
> install on my machine I am willing to merge it. Our Travis builds are not
> yet 100% stable so if they fail but the tests succeed on my machine I
> usually consider the code safe to merge.
>
> That being said, please run some tests with OpenSSH clients/servers that
> support these ciphers as well as some popular applications such as *Putty,
> WinSCP, FileZilla*
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/mina-sshd/pull/132#issuecomment-633253726>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAGBF275RDGO6M5CYXBHUJLRTFBLFANCNFSM4NEHJDRQ>
> .
>
--
Matt Sicker <bo...@gmail.com>
----------------------------------------------------------------
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:
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] jvz commented on a change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r429497101
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseGCMCipher.java
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sshd.common.cipher;
+
+import java.security.spec.AlgorithmParameterSpec;
+
+import javax.crypto.Cipher;
+import javax.crypto.spec.GCMParameterSpec;
+
+import org.apache.sshd.common.util.buffer.Buffer;
+import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
+
+public class BaseGCMCipher extends BaseAEADCipher {
+
+ public BaseGCMCipher(
+ int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation,
+ int blkSize) {
+ super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
+ }
+
+ @Override
+ protected AlgorithmParameterSpec initializeAlgorithmParameters(byte[] iv) {
+ Buffer buffer = new ByteArrayBuffer(iv);
+ buffer.rpos(Integer.BYTES);
+ long ic = buffer.getLong();
+ // decrement IV as it will be incremented back to initial value on first call to updateWithAAD
+ ic = (ic - 1) & 0x0ffffffffL;
+ buffer.wpos(Integer.BYTES);
+ buffer.putLong(ic);
+ return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, buffer.array());
+ }
+
+ @Override
+ protected AlgorithmParameterSpec getNextAlgorithmParameters() {
+ Cipher cipher = getCipherInstance();
+ Buffer iv = new ByteArrayBuffer(cipher.getIV());
+ iv.rpos(Integer.BYTES);
+ long ic = iv.getLong();
+ ic = (ic + 1) & 0x0ffffffffL;
+ iv.wpos(Integer.BYTES);
+ iv.putLong(ic);
+ return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, iv.array());
+ }
Review comment:
I've mostly optimized away all the garbage here. I do an initial IV clone when calling the public `init` method, but that's the only time. I don't see the IV being cloned further by the `Cipher` classes that use `GCMParameterSpec`.
----------------------------------------------------------------
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:
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 #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-650503254
Seems like it is still work in progress - I will hold off testing it and merging into master branch for now
----------------------------------------------------------------
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:
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 change in pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#discussion_r428808221
##########
File path: sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseAEADCipher.java
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.sshd.common.cipher;
+
+import org.apache.sshd.common.util.security.SecurityUtils;
+
+import javax.crypto.Cipher;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.SecretKeySpec;
+import java.security.InvalidAlgorithmParameterException;
+import java.security.InvalidKeyException;
+import java.security.spec.AlgorithmParameterSpec;
+
+public abstract class BaseAEADCipher extends BaseCipher {
+
+ private Mode mode;
+ private SecretKey secretKey;
+ private AlgorithmParameterSpec params;
+ private boolean aadWritten;
+
+ public BaseAEADCipher(
+ int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation, int blkSize) {
+ super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
+ }
+
+ @Override
+ protected byte[] initializeIVData(final Mode mode, final byte[] iv, final int reqLen) {
+ byte[] data = super.initializeIVData(mode, iv, reqLen);
+ params = initializeAlgorithmParameters(data);
+ return data;
+ }
+
+ @Override
+ protected byte[] initializeKeyData(final Mode mode, final byte[] key, final int reqLen) {
+ byte[] keyData = super.initializeKeyData(mode, key, reqLen);
+ secretKey = new SecretKeySpec(keyData, getAlgorithm());
+ return keyData;
+ }
+
+ @Override
+ protected Cipher createCipherInstance(final Mode mode, final byte[] key, final byte[] iv) throws Exception {
+ this.mode = mode;
+ Cipher instance = SecurityUtils.getCipher(getTransformation());
+ init(instance);
+ return instance;
+ }
+
+ private void init(Cipher cipher) throws InvalidKeyException, InvalidAlgorithmParameterException {
Review comment:
Let's make it `protected` in the spirit of open source framework we try and provide our users the greatest possible flexibility in using and modifying our code to their needs. Thus very few pieces of code are truly private.
----------------------------------------------------------------
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:
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 #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-634841278
>> This may be on hold for a week or so as I’m on vacation this week.
Enjoy your vacation - no hurry - when you feel ready I will be happy to review...
----------------------------------------------------------------
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:
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] jvz commented on pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-637256088
Based on what I'm finding online, it does seem like the RFC specifies the all caps version. I'm going to try adding that as well.
----------------------------------------------------------------
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:
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 #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-644251850
>> Yes, I think so. I'm a little iffy on the BaseGCMCipher name for the class as I'm imagining a different use of AES/GCM along the lines of how OpenSSH implements ChaCha20-Poly1305 which would behave differently (the additional authenticated data would be encrypted but not authenticated using another AES key using AES/CTR like how in the packet length is encrypted).
We'll cross that bridge when we come to it - refactoring is not a dirty word in this project
----------------------------------------------------------------
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:
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] jvz commented on pull request #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
jvz commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-643706799
I also tested this with [dropbear](https://matt.ucc.asn.au/dropbear/dropbear.html) so far, all good. Also tested with PuTTY (on macOS).
Feel free to test in any other platforms you're concerned with, but I feel fairly confident with the patch.
----------------------------------------------------------------
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:
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 #132: [SSHD-506] Add support for RFC 5647
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on pull request #132:
URL: https://github.com/apache/mina-sshd/pull/132#issuecomment-665110258
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org