-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
setClientVersion-implementation as discussed in #129 #130
base: master
Are you sure you want to change the base?
Conversation
system property. Added tests.
} | ||
public void setClientVersion(String cv){ | ||
V_C = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will lead to an NPE if a user explicitly calls setClientVersion(null)
because the V_C
property is dereferenced in the connect()
& receive_kexinit()
methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the tests that actually show that require a Docker-container I don't have. I've changed it so that everything is using the get-method now
particular client version has been set
@@ -624,7 +625,7 @@ private KeyExchange receive_kexinit(Buffer buf) throws Exception { | |||
throw new JSchException(e.toString(), e); | |||
} | |||
|
|||
kex.doInit(this, V_S, V_C, I_S, I_C); | |||
kex.doInit(this, V_S, Util.str2byte(getClientVersion()), I_S, I_C); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a user changed the client version string after a Session is connected and a rekeying operation occurred, their SSH connection would then break, because the client version is supposed to be idempotent.
Whilst this could already happen prior, with how you've adapted the Session class to fallback to calling an instance method in the JSch class, we would be making this scenario easier for a user to trigger, since they may naively believe they can change the client version of the JSch instance between multiple calls to getSession()
.
I'm thinking we should avoid this scenario, by simply initializing the V_C
property to whatever the value of jsch.getClientVersion()
is at the time the Session instance is created and avoid trying the callbacks to jsch.getClientVersion()
within session.getClientVersion()
.
We simply would need to document to users that the client version of a Session instance is not affected by changes to the JSch instance after the Session is created, and that the only way to manipulate afterwards is by direct calls to session.setClientVersion()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the client version while being in an active session was possible before so we've found a possible bug here as far as I can see it. I've solved it now by recreating a member V_C that is filled with the bytes of the client version that is returned by getClientVersion
at the time of connect.
"immutable" and allow users to change the setting on the JSch- or Session-level
fillConfig(config, JavaVersion.getVersion()); | ||
} | ||
|
||
static void fillConfig(Map<String, String> config, int javaVersion) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of the javaVersion
argument? It appears to be unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It allows to test the different behavior in dependence of different java versions without the requirement to actually use that particular java version when executing the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's missing is a checkFillConfiig in JschTest but that's a big TODO I haven't had the time to attack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and the actual use of the parameter was missing as well. Not sure what happened here, I've definitely used it when doing the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not sure I understand why we need a javaVersion
argument instead of simply using JavaVersion.getVersion()
?
The value returned by JavaVersion.getVersion()
doesn't dynamically change at runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've committed JschTest showing the use of this parameter as part of a unit test. The fact that JavaVersion.getVersion doesn't change at runtime is the actual problem if you want to test behavior that depends on this value
@@ -81,7 +81,8 @@ | |||
private static final int PACKET_MAX_SIZE = 256 * 1024; | |||
|
|||
private byte[] V_S; // server version | |||
private byte[] V_C=Util.str2byte("SSH-2.0-JSCH_"+JSch.VERSION); // client version | |||
private byte[] V_C; // will be set during connect so that the client version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not simply do VC=Util.str2byte(jsch.getClientVersion())
here?
Then we could omit all the other changes to this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be a setClientVersion on Jsch-level after creating a session and before doing a connect that would get lost that way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's easier to document and understand that calls to jsch.setClientVersion()
have no impact to any previously created Session instances than it is to document and understand that jsch.setClientVersion()
affects any previously created Session instance that has not yet execute session.connect()
.
Basically: after you create a Session object, via jsch.getSession()
, the only way to manipulate the client version string of said Session object should be via session.setClientVersion()
, independent of any calls to jsch.setClientVersion()
that occur after you have created the Session object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we implement it the way you suggested you need to either have a distinct member for the client version bytes used during a connection or check the state of the session and throw an exception if you try to set the version string after a connection has been established.
Otherwise you will run into the problem you've described before:
If a user changed the client version string after a Session is connected and a rekeying
operation occurred, their SSH connection would then break, because the client version
is supposed to be idempotent.
Personally I don't think that this is hard to document and I can add a javadoc comment trying to do it to the method (I would have done so anyway, if the method didn't exist already).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some javadoc that should explain the way this works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still disagree with this.
@mwiede what are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mwiede Any thoughts on this?
Should we just go with a simpler approach I suggested or the more complex approach implemented here?
assertEquals("com.jcraft.jsch.bc.XDH", map.get("xdh"), "check of xdh failed"); | ||
JSch.fillConfig(map, 11); | ||
assertEquals("com.jcraft.jsch.jce.XDH", map.get("xdh"), "check of xdh failed"); | ||
JSch.fillConfig(map, 12); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to test this for 12
since it is the same as 11
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm always testing the edge cases to make sure that if somebody for some reason changes >=
to ==
, the test fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To -v
that: a >= b
can be seen as a shortcut for a == b || a > b
. To have complete coverage over the conditions you need to implement three tests: one for a < b
, one for a == b
and one for a > b
.
assertEquals("com.jcraft.jsch.jce.KeyPairGenEdDSA", map.get("keypairgen.eddsa"), "check of keypairgen.eddsa failed"); | ||
assertEquals("com.jcraft.jsch.jce.SignatureEd25519", map.get("ssh-ed25519"), "check of ssh-ed25519 failed"); | ||
assertEquals("com.jcraft.jsch.jce.SignatureEd448", map.get("ssh-ed448"), "check of ssh-ed448 failed"); | ||
JSch.fillConfig(map, 16); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to test this for 16
since it is the same as 15
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reason as with the test for xdh
void resetJsch() { | ||
JSch.setLogger(null); | ||
orgConfig = (Hashtable<String, String>) JSch.config.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about simply doing new Hashtable<>(JSch.config)
, to avoid the ugly cast and call to clone()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "copy-constructor" creates shallow copies but with immutable values this isn't an issue here, so I've changed it as suggested.
* @return The used version string or <code>null</code> if no | ||
* connection is active. | ||
*/ | ||
public String getUsedClientVersion() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally find this extremely convoluted and would really rather we not add this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since two members are necessary to prevent the effect you've described I though, offering a get-method to allow access to these two members should be available. But I have no special feelings about this method, so I've removed it.
|
Compared to the logger issue this is a more simple change. I've added a system-property that allows to set the client version that way as well and added tests to check the behavior. This required the setup of the config-hashtable to be in a dedicated method.