So IIUC v3 is an unfixable mess but has little real world impact, other than 10% of installs playing Russian roulette with protocol negotiation, but the "bullet" is a fallback to v2?
Looks to me like we can keep this around for now, wait until v4 enabled-by-default reaches a certain threshold (50% perhaps?) and then disable v3 for everyone by default. Or just never disable it by default, and work towards v4 everywhere, with an admin monitor warning admins about agents negotiating an older (v1/v2) protocol (i.e. VersionColumn Plugin light)? > On 10.08.2016, at 16:54, Stephen Connolly <[email protected]> > wrote: > > Thoughts are welcome... > > So as part of my PR for the JNLP4 protocol I have been hitting loads of > issues in the JNLP3 protocol (because I had to refactor the protocol handlers > and thus now can actually test the different protocols via loop-back tests > from within remoting) > > What I have basically found is that the whole negotiation of JNLP3 is riddled > with encoding issues... roundtripping byte arrays as Strings through UTF-8 > encoding. > > I suspect that nobody has hit these issues for two reasons: > > 1. JNLP3 is enabled by default for only 10% of users > 2. If JNLP3 fails, normally JNLP2 will try... and of course that will succeed > > I cannot fix the JNLP3 protocol in a backwards compatible way (as the stream > format does not allow for an up-front versioning so you end up hitting > encoding issues with the encryption of the initial challenge: > > The encypted challenge bytes are encoded as a String in ISO-8859-1, which > gets put into a Properties which gets converted into a String and the > String's UTF-8 bytes then gets sent over the wire... > https://github.com/jenkinsci/remoting/blob/master/src/main/java/org/jenkinsci/remoting/engine/JnlpProtocol3.java#L175 > > Now Java properties encoding is not UTF-8 rather it is ISO-8859-1... so > > Properties props = new Properties(); > props.put(SLAVE_NAME_KEY, slaveName); > props.put(CHALLENGE_KEY, handshakeCiphers.encrypt(challenge)); > if (cookie != null) { > props.put(COOKIE_KEY, handshakeCiphers.encrypt(cookie)); > } > ByteArrayOutputStream o = new ByteArrayOutputStream(); > props.store(o, null); > outputStream.writeUTF(PROTOCOL_PREFIX + NAME); > outputStream.writeUTF(o.toString("UTF-8")); > > Can result in writing an invalid byte sequence in the properties UTF blob... > which then on the server side can result in a corrupted Properties load and > the challenge will fail... > > There are further encoding roundtrip bugs, but as we cannot get past the very > first one there really is not much we can do at all. > > So the question becomes what do we do... > > We can only fix the stream format by breaking backwards compatibility... but > given that JNLP4 is coming, do we even care? > > -Stephen > > > -- > You received this message because you are subscribed to the Google Groups > "Jenkins Developers" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To view this discussion on the web visit > https://groups.google.com/d/msgid/jenkinsci-dev/CA%2BnPnMy9LzgjLaWsXNQVdm7nLbjJKGtnCB_iCWTc-m7UuxFw4w%40mail.gmail.com. > For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/D9D5B8E4-043C-4444-892B-D886FDF26BDA%40beckweb.net. For more options, visit https://groups.google.com/d/optout.
