You are viewing a plain text version of this content. The canonical link for it is here.
Posted to proton@qpid.apache.org by Robbie Gemmell <ro...@gmail.com> on 2015/07/06 15:52:04 UTC
Re: [38/38] qpid-proton git commit: implemented sasl sniffing for
proton-j; this allows the reactor interop tests to pass
Is this change allowing clients to skip the SASL layer when connecting
to servers that have enabled the SASL layer? If so, how is the new
default behaviour disabled?
The existing but unimplemented 'allowSkip' method previously intended
to enable such behaviour still doesn't do anything, so is there a way
to require clients use a SASL layer as would have been previously
after enabling SASL for a proton-j (and in the past a proton-c)
server?
Robbie
On 6 July 2015 at 00:45, <rh...@apache.org> wrote:
> implemented sasl sniffing for proton-j; this allows the reactor interop tests to pass
>
>
> Project: http://git-wip-us.apache.org/repos/asf/qpid-proton/repo
> Commit: http://git-wip-us.apache.org/repos/asf/qpid-proton/commit/46edaebe
> Tree: http://git-wip-us.apache.org/repos/asf/qpid-proton/tree/46edaebe
> Diff: http://git-wip-us.apache.org/repos/asf/qpid-proton/diff/46edaebe
>
> Branch: refs/heads/master
> Commit: 46edaebeb92fa780120f17aa8ac0b54d2feaa8b9
> Parents: bf81c44
> Author: Rafael Schloming <rh...@alum.mit.edu>
> Authored: Sun Jul 5 19:08:20 2015 -0400
> Committer: Rafael Schloming <rh...@alum.mit.edu>
> Committed: Sun Jul 5 19:33:45 2015 -0400
>
> ----------------------------------------------------------------------
> proton-c/bindings/python/proton/__init__.py | 2 +-
> .../impl/HandshakeSniffingTransportWrapper.java | 182 +++++++++++++++++++
> .../qpid/proton/engine/impl/SaslImpl.java | 12 +-
> .../qpid/proton/engine/impl/SaslSniffer.java | 53 ++++++
> .../SslHandshakeSniffingTransportWrapper.java | 170 +++--------------
> proton-j/src/main/resources/cengine.py | 26 ++-
> 6 files changed, 284 insertions(+), 161 deletions(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/46edaebe/proton-c/bindings/python/proton/__init__.py
> ----------------------------------------------------------------------
> diff --git a/proton-c/bindings/python/proton/__init__.py b/proton-c/bindings/python/proton/__init__.py
> index 1032edf..9c75800 100644
> --- a/proton-c/bindings/python/proton/__init__.py
> +++ b/proton-c/bindings/python/proton/__init__.py
> @@ -3294,7 +3294,7 @@ A callback for trace logging. The callback is passed the transport and log messa
> def push(self, binary):
> n = self._check(pn_transport_push(self._impl, binary))
> if n != len(binary):
> - raise OverflowError("unable to process all bytes")
> + raise OverflowError("unable to process all bytes: %s, %s" % (n, len(binary)))
>
> def close_tail(self):
> self._check(pn_transport_close_tail(self._impl))
>
> http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/46edaebe/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/HandshakeSniffingTransportWrapper.java
> ----------------------------------------------------------------------
> diff --git a/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/HandshakeSniffingTransportWrapper.java b/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/HandshakeSniffingTransportWrapper.java
> new file mode 100644
> index 0000000..6a5aac5
> --- /dev/null
> +++ b/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/HandshakeSniffingTransportWrapper.java
> @@ -0,0 +1,182 @@
> +/*
> + *
> + * 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.qpid.proton.engine.impl;
> +
> +import java.nio.ByteBuffer;
> +
> +import org.apache.qpid.proton.engine.Transport;
> +import org.apache.qpid.proton.engine.TransportException;
> +import org.apache.qpid.proton.engine.impl.TransportWrapper;
> +
> +public abstract class HandshakeSniffingTransportWrapper<T1 extends TransportWrapper, T2 extends TransportWrapper>
> + implements TransportWrapper
> +{
> +
> + protected final T1 _wrapper1;
> + protected final T2 _wrapper2;
> +
> + private boolean _tail_closed = false;
> + private boolean _head_closed = false;
> + protected TransportWrapper _selectedTransportWrapper;
> +
> + private final ByteBuffer _determinationBuffer;
> +
> + protected HandshakeSniffingTransportWrapper
> + (T1 wrapper1,
> + T2 wrapper2)
> + {
> + _wrapper1 = wrapper1;
> + _wrapper2 = wrapper2;
> + _determinationBuffer = ByteBuffer.allocate(bufferSize());
> + }
> +
> + @Override
> + public int capacity()
> + {
> + if (isDeterminationMade())
> + {
> + return _selectedTransportWrapper.capacity();
> + }
> + else
> + {
> + if (_tail_closed) { return Transport.END_OF_STREAM; }
> + return _determinationBuffer.remaining();
> + }
> + }
> +
> + @Override
> + public int position()
> + {
> + if (isDeterminationMade())
> + {
> + return _selectedTransportWrapper.position();
> + }
> + else
> + {
> + if (_tail_closed) { return Transport.END_OF_STREAM; }
> + return _determinationBuffer.position();
> + }
> + }
> +
> + @Override
> + public ByteBuffer tail()
> + {
> + if (isDeterminationMade())
> + {
> + return _selectedTransportWrapper.tail();
> + }
> + else
> + {
> + return _determinationBuffer;
> + }
> + }
> +
> + protected abstract int bufferSize();
> +
> + protected abstract void makeDetermination(byte[] bytes);
> +
> + @Override
> + public void process() throws TransportException
> + {
> + if (isDeterminationMade())
> + {
> + _selectedTransportWrapper.process();
> + }
> + else if (_determinationBuffer.remaining() == 0)
> + {
> + _determinationBuffer.flip();
> + byte[] bytesInput = new byte[_determinationBuffer.remaining()];
> + _determinationBuffer.get(bytesInput);
> + makeDetermination(bytesInput);
> + _determinationBuffer.rewind();
> +
> + // TODO what if the selected transport has insufficient capacity?? Maybe use pour, and then try to finish pouring next time round.
> + _selectedTransportWrapper.tail().put(_determinationBuffer);
> + _selectedTransportWrapper.process();
> + } else if (_tail_closed) {
> + throw new TransportException("connection aborted");
> + }
> + }
> +
> + @Override
> + public void close_tail()
> + {
> + try {
> + if (isDeterminationMade())
> + {
> + _selectedTransportWrapper.close_tail();
> + }
> + } finally {
> + _tail_closed = true;
> + }
> + }
> +
> + @Override
> + public int pending()
> + {
> + if (_head_closed) { return Transport.END_OF_STREAM; }
> +
> + if (isDeterminationMade()) {
> + return _selectedTransportWrapper.pending();
> + } else {
> + return 0;
> + }
> +
> + }
> +
> + private static final ByteBuffer EMPTY = ByteBuffer.allocate(0);
> +
> + @Override
> + public ByteBuffer head()
> + {
> + if (isDeterminationMade()) {
> + return _selectedTransportWrapper.head();
> + } else {
> + return EMPTY;
> + }
> + }
> +
> + @Override
> + public void pop(int bytes)
> + {
> + if (isDeterminationMade()) {
> + _selectedTransportWrapper.pop(bytes);
> + } else if (bytes > 0) {
> + throw new IllegalStateException("no bytes have been read");
> + }
> + }
> +
> + @Override
> + public void close_head()
> + {
> + if (isDeterminationMade()) {
> + _selectedTransportWrapper.close_head();
> + } else {
> + _head_closed = true;
> + }
> + }
> +
> + protected boolean isDeterminationMade()
> + {
> + return _selectedTransportWrapper != null;
> + }
> +
> +}
>
> http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/46edaebe/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/SaslImpl.java
> ----------------------------------------------------------------------
> diff --git a/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/SaslImpl.java b/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/SaslImpl.java
> index 053c21f..6efb140 100644
> --- a/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/SaslImpl.java
> +++ b/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/SaslImpl.java
> @@ -485,7 +485,17 @@ public class SaslImpl implements Sasl, SaslFrameBody.SaslFrameBodyHandler<Void>,
>
> public TransportWrapper wrap(final TransportInput input, final TransportOutput output)
> {
> - return new SaslTransportWrapper(input, output);
> + return new SaslSniffer(new SaslTransportWrapper(input, output),
> + new PlainTransportWrapper(output, input)) {
> + protected boolean isDeterminationMade() {
> + if (_role == Role.SERVER) {
> + return super.isDeterminationMade();
> + } else {
> + _selectedTransportWrapper = _wrapper1;
> + return true;
> + }
> + }
> + };
> }
>
> @Override
>
> http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/46edaebe/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/SaslSniffer.java
> ----------------------------------------------------------------------
> diff --git a/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/SaslSniffer.java b/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/SaslSniffer.java
> new file mode 100644
> index 0000000..2d92496
> --- /dev/null
> +++ b/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/SaslSniffer.java
> @@ -0,0 +1,53 @@
> +/*
> + *
> + * 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.qpid.proton.engine.impl;
> +
> +
> +/**
> + * SaslSniffer
> + *
> + */
> +
> +class SaslSniffer extends HandshakeSniffingTransportWrapper<TransportWrapper, TransportWrapper>
> +{
> +
> + SaslSniffer(TransportWrapper sasl, TransportWrapper other) {
> + super(sasl, other);
> + }
> +
> + protected int bufferSize() { return AmqpHeader.SASL_HEADER.length; }
> +
> + protected void makeDetermination(byte[] bytes) {
> + if (bytes.length < bufferSize()) {
> + throw new IllegalArgumentException("insufficient bytes");
> + }
> +
> + for (int i = 0; i < AmqpHeader.SASL_HEADER.length; i++) {
> + if (bytes[i] != AmqpHeader.SASL_HEADER[i]) {
> + _selectedTransportWrapper = _wrapper2;
> + return;
> + }
> + }
> +
> + _selectedTransportWrapper = _wrapper1;
> + }
> +
> +}
>
> http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/46edaebe/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/ssl/SslHandshakeSniffingTransportWrapper.java
> ----------------------------------------------------------------------
> diff --git a/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/ssl/SslHandshakeSniffingTransportWrapper.java b/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/ssl/SslHandshakeSniffingTransportWrapper.java
> index 1efc7a9..c678343 100644
> --- a/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/ssl/SslHandshakeSniffingTransportWrapper.java
> +++ b/proton-j/src/main/java/org/apache/qpid/proton/engine/impl/ssl/SslHandshakeSniffingTransportWrapper.java
> @@ -20,150 +20,21 @@
> */
> package org.apache.qpid.proton.engine.impl.ssl;
>
> -import java.nio.ByteBuffer;
> -
> -import org.apache.qpid.proton.engine.Transport;
> -import org.apache.qpid.proton.engine.TransportException;
> +import org.apache.qpid.proton.engine.impl.HandshakeSniffingTransportWrapper;
> import org.apache.qpid.proton.engine.impl.TransportWrapper;
>
> -public class SslHandshakeSniffingTransportWrapper implements SslTransportWrapper
> -{
> - private static final int MINIMUM_LENGTH_FOR_DETERMINATION = 5;
> - private final SslTransportWrapper _secureTransportWrapper;
> - private final TransportWrapper _plainTransportWrapper;
> -
> - private boolean _tail_closed = false;
> - private boolean _head_closed = false;
> - private TransportWrapper _selectedTransportWrapper;
> -
> - private final ByteBuffer _determinationBuffer = ByteBuffer.allocate(MINIMUM_LENGTH_FOR_DETERMINATION);
> -
> - SslHandshakeSniffingTransportWrapper
> - (SslTransportWrapper secureTransportWrapper,
> - TransportWrapper plainTransportWrapper)
> - {
> - _secureTransportWrapper = secureTransportWrapper;
> - _plainTransportWrapper = plainTransportWrapper;
> - }
> -
> - @Override
> - public int capacity()
> - {
> - if (isDeterminationMade())
> - {
> - return _selectedTransportWrapper.capacity();
> - }
> - else
> - {
> - if (_tail_closed) { return Transport.END_OF_STREAM; }
> - return _determinationBuffer.remaining();
> - }
> - }
> -
> - @Override
> - public int position()
> - {
> - if (isDeterminationMade())
> - {
> - return _selectedTransportWrapper.position();
> - }
> - else
> - {
> - if (_tail_closed) { return Transport.END_OF_STREAM; }
> - return _determinationBuffer.position();
> - }
> - }
> -
> - @Override
> - public ByteBuffer tail()
> - {
> - if (isDeterminationMade())
> - {
> - return _selectedTransportWrapper.tail();
> - }
> - else
> - {
> - return _determinationBuffer;
> - }
> - }
> -
> - @Override
> - public void process() throws TransportException
> - {
> - if (isDeterminationMade())
> - {
> - _selectedTransportWrapper.process();
> - }
> - else if (_determinationBuffer.remaining() == 0)
> - {
> - _determinationBuffer.flip();
> - byte[] bytesInput = new byte[_determinationBuffer.remaining()];
> - _determinationBuffer.get(bytesInput);
> - makeSslDetermination(bytesInput);
> - _determinationBuffer.rewind();
> -
> - // TODO what if the selected transport has insufficient capacity?? Maybe use pour, and then try to finish pouring next time round.
> - _selectedTransportWrapper.tail().put(_determinationBuffer);
> - _selectedTransportWrapper.process();
> - } else if (_tail_closed) {
> - throw new TransportException("connection aborted");
> - }
> - }
> -
> - @Override
> - public void close_tail()
> - {
> - try {
> - if (isDeterminationMade())
> - {
> - _selectedTransportWrapper.close_tail();
> - }
> - } finally {
> - _tail_closed = true;
> - }
> - }
> -
> - @Override
> - public int pending()
> - {
> - if (_head_closed) { return Transport.END_OF_STREAM; }
> -
> - if (isDeterminationMade()) {
> - return _selectedTransportWrapper.pending();
> - } else {
> - return 0;
> - }
> -
> - }
>
> - @Override
> - public ByteBuffer head()
> - {
> - if (isDeterminationMade()) {
> - return _selectedTransportWrapper.head();
> - } else {
> - return null;
> - }
> - }
> +/**
> + * SslHandshakeSniffingTransportWrapper
> + *
> + */
>
> - @Override
> - public void pop(int bytes)
> - {
> - if (isDeterminationMade()) {
> - _selectedTransportWrapper.pop(bytes);
> - } else if (bytes > 0) {
> - throw new IllegalStateException("no bytes have been read");
> - }
> - }
> +public class SslHandshakeSniffingTransportWrapper extends HandshakeSniffingTransportWrapper<SslTransportWrapper, TransportWrapper>
> + implements SslTransportWrapper
> +{
>
> - @Override
> - public void close_head()
> - {
> - if (isDeterminationMade()) {
> - _selectedTransportWrapper.close_head();
> - } else {
> - _head_closed = true;
> - }
> + SslHandshakeSniffingTransportWrapper(SslTransportWrapper ssl, TransportWrapper plain) {
> + super(ssl, plain);
> }
>
> @Override
> @@ -171,7 +42,7 @@ public class SslHandshakeSniffingTransportWrapper implements SslTransportWrapper
> {
> if(isSecureWrapperSelected())
> {
> - return _secureTransportWrapper.getCipherName();
> + return _wrapper1.getCipherName();
> }
> else
> {
> @@ -185,7 +56,7 @@ public class SslHandshakeSniffingTransportWrapper implements SslTransportWrapper
> {
> if (isSecureWrapperSelected())
> {
> - return _secureTransportWrapper.getProtocolName();
> + return _wrapper1.getProtocolName();
> }
> else
> {
> @@ -195,32 +66,33 @@ public class SslHandshakeSniffingTransportWrapper implements SslTransportWrapper
>
> private boolean isSecureWrapperSelected()
> {
> - return _selectedTransportWrapper == _secureTransportWrapper;
> + return _selectedTransportWrapper == _wrapper1;
> }
>
> - private boolean isDeterminationMade()
> - {
> - return _selectedTransportWrapper != null;
> + protected int bufferSize() {
> + // minimum length for determination
> + return 5;
> }
>
> - private void makeSslDetermination(byte[] bytesInput)
> + protected void makeDetermination(byte[] bytesInput)
> {
> boolean isSecure = checkForSslHandshake(bytesInput);
> if (isSecure)
> {
> - _selectedTransportWrapper = _secureTransportWrapper;
> + _selectedTransportWrapper = _wrapper1;
> }
> else
> {
> - _selectedTransportWrapper = _plainTransportWrapper;
> + _selectedTransportWrapper = _wrapper2;
> }
> }
> +
> // TODO perhaps the sniffer should save up the bytes from each
> // input call until it has sufficient bytes to make the determination
> // and only then pass them to the secure or plain wrapped transport?
> private boolean checkForSslHandshake(byte[] buf)
> {
> - if (buf.length >= MINIMUM_LENGTH_FOR_DETERMINATION)
> + if (buf.length >= bufferSize())
> {
> /*
> * SSLv2 Client Hello format
>
> http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/46edaebe/proton-j/src/main/resources/cengine.py
> ----------------------------------------------------------------------
> diff --git a/proton-j/src/main/resources/cengine.py b/proton-j/src/main/resources/cengine.py
> index 1e89220..c94d023 100644
> --- a/proton-j/src/main/resources/cengine.py
> +++ b/proton-j/src/main/resources/cengine.py
> @@ -940,16 +940,22 @@ def pn_transport_capacity(trans):
> return trans.impl.capacity()
>
> def pn_transport_push(trans, input):
> - cap = pn_transport_capacity(trans)
> - if cap < 0:
> - return cap
> - elif len(input) > cap:
> - input = input[:cap]
> -
> - bb = trans.impl.tail()
> - bb.put(array(input, 'b'))
> - trans.impl.process()
> - return len(input)
> + result = 0
> + while input:
> + cap = pn_transport_capacity(trans)
> + if cap < 0:
> + return cap
> + elif len(input) > cap:
> + trimmed = input[:cap]
> + else:
> + trimmed = input
> +
> + bb = trans.impl.tail()
> + bb.put(array(trimmed, 'b'))
> + trans.impl.process()
> + input = input[cap:]
> + result += len(trimmed)
> + return result
>
> def pn_transport_close_head(trans):
> trans.impl.close_head()
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
> For additional commands, e-mail: commits-help@qpid.apache.org
>
Re: [38/38] qpid-proton git commit: implemented sasl sniffing for
proton-j; this allows the reactor interop tests to pass
Posted by Robbie Gemmell <ro...@gmail.com>.
On 6 July 2015 at 18:14, Andrew Stitcher <as...@redhat.com> wrote:
> On Mon, 2015-07-06 at 17:48 +0100, Robbie Gemmell wrote:
>> ...
>> The old toggle only used to define whether sasl was required or not
>> (which it historically was once you enabled the sasl layer, and the
>> toggle was never implemented in proton-j), whereas IIRC the new
>> 'requireAuth' governs that but also whether ANONYMOUS is allowed or
>> not when a SASL layer is used, is that correct?
>
> That is true, but I think it actually more useful to be able to select
> authenticated or not compared to using SASL or not (because ANONYMOUS is
> unauthenticated but uses SASL).
>
I wasn't arguing one way or another what is better here, I was just
pointing out the difference given its important to this
change-right-before-release and that proton-j doesn't have an updated
SASL API/Impl to go along with it.
> The C implementation does the actual enforcement when it reads the AMQP
> header, which would obviously be a significant change to the Java
> implementation, but I really do think gives a more satisfactory user
> result.
>
>> Given the older SASL
>> API means much of the SASL work including mechansim selection happens
>> outside/before proton-j and you get as far as making a connection,
>> isnt that going to be a bit of a pain to implement and use?
>
> Well I obviously disagree, else I would have implemented the C SASL
> support like this!
If it wasnt clear above, I only meant it might be pain to implement
and use isAuthenticated right now *given the older SASL API/Impl* that
currently still exists in proton-j and presumably isnt going away
before the next release.
>
> TBH, I think that using the same API in Java would make sense along with
> an extra "plugin API" to allow implementing mechanisms (assuming there
> is no external Java library like Cyrus SASL that can do the work for
> us).
>
> But yes that is new work, probably not going to happen before Proton
> 0.11.
>>
>
>> Might it be simpler to use allowSkip() until such time the wider SASL
>> API is actually implemented, keep the default behaviour as it was
>> previously (so people will be less likely to need to use the behaviour
>> toggle and less likely to care what it is called), and update
>> whichever reactor tests that needed the change to configure it that
>> way in the meantime?
>
> I'm agnostic on this point really, I'd just like to ensure that the C
> and Java APIs actually converge in the shortest time scale.
My comment above was coming from an impression that the change here
wouldn't be close to fully aligning them, meaning further change and
probable breakage would be required down the line. If thats the case I
would tend towards leaving the existing default behaviour until such
time a viable attempt at converging them is actually being made, in
which case such change and breakage would be better warranted and
rewarded.
> The
> alternative would be to stop testing them with the same test code -
> there are already far too many specific tests for Java or Windows in the
> tests.
>
>From my impression jsut as many of the specific tests are C or Linux
specific given they often [now] test things that simply haven't been
implemented elsewhere yet :)
I'll admit I have writetn some Java specific tests in actual Java on
occasion....sometimes because I burned a lot of time first trying to
write it in python and found the test shim stuff entirely masked the
problem so I actually couldn't, and others just because it was a known
handful of minutes as a direct pure unit test versus indeterminate
time working out how the to do it far less directly via the python
tests that I simply justify at the time.
Re: [38/38] qpid-proton git commit: implemented sasl sniffing for
proton-j; this allows the reactor interop tests to pass
Posted by Robbie Gemmell <ro...@gmail.com>.
On 6 July 2015 at 18:57, Rafael Schloming <rh...@alum.mit.edu> wrote:
> FWIW, my changes in this area really represent the minimum diff necessary
> to get the reactor branch to land. None of this is related to the reactor
> changes per se,
Yes, it almost seems like a change in default behaviour of a sasl
enabled server to allow non-SASL connections should have a JIRA of its
own :P
> it just so happens the reactor tests include several tests
> that check interop between proton-c and proton-j and these tests keep
> stumbling over incompatibilities that are currently quite easy to arise
> given the current state of the sasl implementations.
>
> While I agree 100% that the APIs should converge, at the moment I'm
> actually slightly more worried about the on-the-wire interop issues. More
> specifically, while it's bad for proton-c and proton-j APIs to look
> different, it's *really* bad if the default settings for one result in a
> configuration that won't interop with the default settings for the other.
>
> --Rafael
I can agree with that. Although I do feel the defaults for both
engines (independent from the defaults of any of our other code that
uses them, which can do anything it likes) are now wrong as a result
of this latest change ;)
>
> On Mon, Jul 6, 2015 at 10:28 AM, Andrew Stitcher <as...@redhat.com>
> wrote:
>
>> On Mon, 2015-07-06 at 13:14 -0400, Andrew Stitcher wrote:
>> > On Mon, 2015-07-06 at 17:48 +0100, Robbie Gemmell wrote:
>> > > ...
>> > > The old toggle only used to define whether sasl was required or not
>> > > (which it historically was once you enabled the sasl layer, and the
>> > > toggle was never implemented in proton-j), whereas IIRC the new
>> > > 'requireAuth' governs that but also whether ANONYMOUS is allowed or
>> > > not when a SASL layer is used, is that correct?
>> >
>> > That is true, but I think it actually more useful to be able to select
>> > authenticated or not compared to using SASL or not (because ANONYMOUS is
>> > unauthenticated but uses SASL).
>> >
>> > The C implementation does the actual enforcement when it reads the AMQP
>> > header, which would obviously be a significant change to the Java
>> > implementation, but I really do think gives a more satisfactory user
>> > result.
>>
>> The reason for the complexity and the checking at AMQP header time is to
>> allow SSL certificates as a valid form of authentication (not
>> necessarily only used with SASL EXTERNAL). If you don't need to support
>> that (or at least not yet) then "require authentication" can simply mean
>> require the SASL layer but don't offer the ANONYMOUS mechanism. That is
>> what earlier versions of the C code did*, and I think that would be
>> relatively simple to implement in Java too.
>>
>> * The C code will still not offer ANONYMOUS as a possible mechanism if
>> authentication is required. But the overall meaning of the flag is more
>> complex than this as explained.
>>
>> Andrew
>>
>>
>>
Re: [38/38] qpid-proton git commit: implemented sasl sniffing for
proton-j; this allows the reactor interop tests to pass
Posted by Rafael Schloming <rh...@alum.mit.edu>.
FWIW, my changes in this area really represent the minimum diff necessary
to get the reactor branch to land. None of this is related to the reactor
changes per se, it just so happens the reactor tests include several tests
that check interop between proton-c and proton-j and these tests keep
stumbling over incompatibilities that are currently quite easy to arise
given the current state of the sasl implementations.
While I agree 100% that the APIs should converge, at the moment I'm
actually slightly more worried about the on-the-wire interop issues. More
specifically, while it's bad for proton-c and proton-j APIs to look
different, it's *really* bad if the default settings for one result in a
configuration that won't interop with the default settings for the other.
--Rafael
On Mon, Jul 6, 2015 at 10:28 AM, Andrew Stitcher <as...@redhat.com>
wrote:
> On Mon, 2015-07-06 at 13:14 -0400, Andrew Stitcher wrote:
> > On Mon, 2015-07-06 at 17:48 +0100, Robbie Gemmell wrote:
> > > ...
> > > The old toggle only used to define whether sasl was required or not
> > > (which it historically was once you enabled the sasl layer, and the
> > > toggle was never implemented in proton-j), whereas IIRC the new
> > > 'requireAuth' governs that but also whether ANONYMOUS is allowed or
> > > not when a SASL layer is used, is that correct?
> >
> > That is true, but I think it actually more useful to be able to select
> > authenticated or not compared to using SASL or not (because ANONYMOUS is
> > unauthenticated but uses SASL).
> >
> > The C implementation does the actual enforcement when it reads the AMQP
> > header, which would obviously be a significant change to the Java
> > implementation, but I really do think gives a more satisfactory user
> > result.
>
> The reason for the complexity and the checking at AMQP header time is to
> allow SSL certificates as a valid form of authentication (not
> necessarily only used with SASL EXTERNAL). If you don't need to support
> that (or at least not yet) then "require authentication" can simply mean
> require the SASL layer but don't offer the ANONYMOUS mechanism. That is
> what earlier versions of the C code did*, and I think that would be
> relatively simple to implement in Java too.
>
> * The C code will still not offer ANONYMOUS as a possible mechanism if
> authentication is required. But the overall meaning of the flag is more
> complex than this as explained.
>
> Andrew
>
>
>
Re: [38/38] qpid-proton git commit: implemented sasl sniffing for
proton-j; this allows the reactor interop tests to pass
Posted by Robbie Gemmell <ro...@gmail.com>.
On 6 July 2015 at 18:28, Andrew Stitcher <as...@redhat.com> wrote:
> On Mon, 2015-07-06 at 13:14 -0400, Andrew Stitcher wrote:
>> On Mon, 2015-07-06 at 17:48 +0100, Robbie Gemmell wrote:
>> > ...
>> > The old toggle only used to define whether sasl was required or not
>> > (which it historically was once you enabled the sasl layer, and the
>> > toggle was never implemented in proton-j), whereas IIRC the new
>> > 'requireAuth' governs that but also whether ANONYMOUS is allowed or
>> > not when a SASL layer is used, is that correct?
>>
>> That is true, but I think it actually more useful to be able to select
>> authenticated or not compared to using SASL or not (because ANONYMOUS is
>> unauthenticated but uses SASL).
>>
>> The C implementation does the actual enforcement when it reads the AMQP
>> header, which would obviously be a significant change to the Java
>> implementation, but I really do think gives a more satisfactory user
>> result.
>
> The reason for the complexity and the checking at AMQP header time is to
> allow SSL certificates as a valid form of authentication (not
> necessarily only used with SASL EXTERNAL). If you don't need to support
> that (or at least not yet) then "require authentication" can simply mean
> require the SASL layer but don't offer the ANONYMOUS mechanism. That is
> what earlier versions of the C code did*, and I think that would be
> relatively simple to implement in Java too.
>
> * The C code will still not offer ANONYMOUS as a possible mechanism if
> authentication is required. But the overall meaning of the flag is more
> complex than this as explained.
>
> Andrew
>
Yeah, I appreciated that, I just didn't think it necessary detail for
the fairly basic point I was trying to make. As mentioned in my last
mail, I was really just pointing out the mismatch between the existing
behaviour and what requireAuthentication and isAuthenitcated do versus
the lack of the whole new SASL/auth API/impl in proton-j at this point
in time, possibly days before release, which seems to be needed in
order to make them actually work the way they do in proton-c.
If all we do now is change the default behaviour a little now in a way
that requries most people to update their existing usage, knowing we
will change the same bits again in bigger way that will require most
people to update their already-updated usage again, then I'd prefer we
didnt change the default behaviour now as the near term reward doesnt
seem that great for all the ongoing hassle from the repeated changes,
thats all I was meaning.
Re: [38/38] qpid-proton git commit: implemented sasl sniffing for
proton-j; this allows the reactor interop tests to pass
Posted by Andrew Stitcher <as...@redhat.com>.
On Mon, 2015-07-06 at 13:14 -0400, Andrew Stitcher wrote:
> On Mon, 2015-07-06 at 17:48 +0100, Robbie Gemmell wrote:
> > ...
> > The old toggle only used to define whether sasl was required or not
> > (which it historically was once you enabled the sasl layer, and the
> > toggle was never implemented in proton-j), whereas IIRC the new
> > 'requireAuth' governs that but also whether ANONYMOUS is allowed or
> > not when a SASL layer is used, is that correct?
>
> That is true, but I think it actually more useful to be able to select
> authenticated or not compared to using SASL or not (because ANONYMOUS is
> unauthenticated but uses SASL).
>
> The C implementation does the actual enforcement when it reads the AMQP
> header, which would obviously be a significant change to the Java
> implementation, but I really do think gives a more satisfactory user
> result.
The reason for the complexity and the checking at AMQP header time is to
allow SSL certificates as a valid form of authentication (not
necessarily only used with SASL EXTERNAL). If you don't need to support
that (or at least not yet) then "require authentication" can simply mean
require the SASL layer but don't offer the ANONYMOUS mechanism. That is
what earlier versions of the C code did*, and I think that would be
relatively simple to implement in Java too.
* The C code will still not offer ANONYMOUS as a possible mechanism if
authentication is required. But the overall meaning of the flag is more
complex than this as explained.
Andrew
Re: [38/38] qpid-proton git commit: implemented sasl sniffing for
proton-j; this allows the reactor interop tests to pass
Posted by Andrew Stitcher <as...@redhat.com>.
On Mon, 2015-07-06 at 17:48 +0100, Robbie Gemmell wrote:
> ...
> The old toggle only used to define whether sasl was required or not
> (which it historically was once you enabled the sasl layer, and the
> toggle was never implemented in proton-j), whereas IIRC the new
> 'requireAuth' governs that but also whether ANONYMOUS is allowed or
> not when a SASL layer is used, is that correct?
That is true, but I think it actually more useful to be able to select
authenticated or not compared to using SASL or not (because ANONYMOUS is
unauthenticated but uses SASL).
The C implementation does the actual enforcement when it reads the AMQP
header, which would obviously be a significant change to the Java
implementation, but I really do think gives a more satisfactory user
result.
> Given the older SASL
> API means much of the SASL work including mechansim selection happens
> outside/before proton-j and you get as far as making a connection,
> isnt that going to be a bit of a pain to implement and use?
Well I obviously disagree, else I would have implemented the C SASL
support like this!
TBH, I think that using the same API in Java would make sense along with
an extra "plugin API" to allow implementing mechanisms (assuming there
is no external Java library like Cyrus SASL that can do the work for
us).
But yes that is new work, probably not going to happen before Proton
0.11.
>
> Might it be simpler to use allowSkip() until such time the wider SASL
> API is actually implemented, keep the default behaviour as it was
> previously (so people will be less likely to need to use the behaviour
> toggle and less likely to care what it is called), and update
> whichever reactor tests that needed the change to configure it that
> way in the meantime?
I'm agnostic on this point really, I'd just like to ensure that the C
and Java APIs actually converge in the shortest time scale. The
alternative would be to stop testing them with the same test code -
there are already far too many specific tests for Java or Windows in the
tests.
Andrew
Re: [38/38] qpid-proton git commit: implemented sasl sniffing for
proton-j; this allows the reactor interop tests to pass
Posted by Robbie Gemmell <ro...@gmail.com>.
On 6 July 2015 at 16:51, Andrew Stitcher <as...@redhat.com> wrote:
> On Mon, 2015-07-06 at 11:30 -0400, Rafael Schloming wrote:
>> I wired in allowSkip in a very minimal way just to restore the ability to
>> force the old behaviour. It would be a fairly trivial to change the name of
>> course,
>
> I'm not sure if that really applies as the method is on a different
> object now (transport instead of SASL).
>
>> however it appears there are a bunch of other related changes that
>> go along with it, e.g. adding a bunch of accessors and fixing the error
>> behavior. Currently if you put in require authentication the java sasl
>> layer will simply die with a TransportException if it sees a regular AMQP
>> header, and the tests appear to expect something more graceful.
>
> Yes you will need the isAuthenticated property.
>
The old toggle only used to define whether sasl was required or not
(which it historically was once you enabled the sasl layer, and the
toggle was never implemented in proton-j), whereas IIRC the new
'requireAuth' governs that but also whether ANONYMOUS is allowed or
not when a SASL layer is used, is that correct? Given the older SASL
API means much of the SASL work including mechansim selection happens
outside/before proton-j and you get as far as making a connection,
isnt that going to be a bit of a pain to implement and use?
Might it be simpler to use allowSkip() until such time the wider SASL
API is actually implemented, keep the default behaviour as it was
previously (so people will be less likely to need to use the behaviour
toggle and less likely to care what it is called), and update
whichever reactor tests that needed the change to configure it that
way in the meantime?
> Hmm, not sure about that, I think the tests just ensure that
> authenticated is not true, although I suppose they do rely on not
> getting an exception, that may be incidental behaviour though.
>
> The new APIs aren't very extensive, and should be fairly extensively
> documented at https://cwiki.apache.org/confluence/x/B5cWAw
> (this should be up to date).
>
> Bringing Proton-J in line with the new SASL APIs is
> https://issues.apache.org/jira/browse/PROTON-910
>
> Andrew
>
>
Re: [38/38] qpid-proton git commit: implemented sasl sniffing for
proton-j; this allows the reactor interop tests to pass
Posted by Andrew Stitcher <as...@redhat.com>.
On Mon, 2015-07-06 at 11:30 -0400, Rafael Schloming wrote:
> I wired in allowSkip in a very minimal way just to restore the ability to
> force the old behaviour. It would be a fairly trivial to change the name of
> course,
I'm not sure if that really applies as the method is on a different
object now (transport instead of SASL).
> however it appears there are a bunch of other related changes that
> go along with it, e.g. adding a bunch of accessors and fixing the error
> behavior. Currently if you put in require authentication the java sasl
> layer will simply die with a TransportException if it sees a regular AMQP
> header, and the tests appear to expect something more graceful.
Yes you will need the isAuthenticated property.
Hmm, not sure about that, I think the tests just ensure that
authenticated is not true, although I suppose they do rely on not
getting an exception, that may be incidental behaviour though.
The new APIs aren't very extensive, and should be fairly extensively
documented at https://cwiki.apache.org/confluence/x/B5cWAw
(this should be up to date).
Bringing Proton-J in line with the new SASL APIs is
https://issues.apache.org/jira/browse/PROTON-910
Andrew
Re: [38/38] qpid-proton git commit: implemented sasl sniffing for
proton-j; this allows the reactor interop tests to pass
Posted by Rafael Schloming <rh...@alum.mit.edu>.
I wired in allowSkip in a very minimal way just to restore the ability to
force the old behaviour. It would be a fairly trivial to change the name of
course, however it appears there are a bunch of other related changes that
go along with it, e.g. adding a bunch of accessors and fixing the error
behavior. Currently if you put in require authentication the java sasl
layer will simply die with a TransportException if it sees a regular AMQP
header, and the tests appear to expect something more graceful.
I stopped there because I noticed a bunch of other unimplemented stuff and
I wasn't sure how deep the bottom of the rabbit hole was.
--Rafael
On Mon, Jul 6, 2015 at 11:11 AM, Andrew Stitcher <as...@redhat.com>
wrote:
> On Mon, 2015-07-06 at 10:56 -0400, Rafael Schloming wrote:
> > On Mon, Jul 6, 2015 at 9:52 AM, Robbie Gemmell <robbie.gemmell@gmail.com
> >
> > wrote:
> >
> > > Is this change allowing clients to skip the SASL layer when connecting
> > > to servers that have enabled the SASL layer? If so, how is the new
> > > default behaviour disabled?
> > >
> >
> > Yes, it was necessary to allow the tests to pass.
> >
> >
> > > The existing but unimplemented 'allowSkip' method previously intended
> > > to enable such behaviour still doesn't do anything, so is there a way
> > > to require clients use a SASL layer as would have been previously
> > > after enabling SASL for a proton-j (and in the past a proton-c)
> > > server?
> > >
> >
> > Ah, I didn't notice that. Thanks for pointing it out. I'll wire it up and
> > cross my fingers that the tests still pass.
>
> Allow_skip is no longer present in the C API it is replaced with the
> require_auth (now on the transport object) API.
>
> So it would make more sense to implement that and remove allow_skip.
>
> >
> > --Rafael
>
>
>
Re: [38/38] qpid-proton git commit: implemented sasl sniffing for
proton-j; this allows the reactor interop tests to pass
Posted by Andrew Stitcher <as...@redhat.com>.
On Mon, 2015-07-06 at 10:56 -0400, Rafael Schloming wrote:
> On Mon, Jul 6, 2015 at 9:52 AM, Robbie Gemmell <ro...@gmail.com>
> wrote:
>
> > Is this change allowing clients to skip the SASL layer when connecting
> > to servers that have enabled the SASL layer? If so, how is the new
> > default behaviour disabled?
> >
>
> Yes, it was necessary to allow the tests to pass.
>
>
> > The existing but unimplemented 'allowSkip' method previously intended
> > to enable such behaviour still doesn't do anything, so is there a way
> > to require clients use a SASL layer as would have been previously
> > after enabling SASL for a proton-j (and in the past a proton-c)
> > server?
> >
>
> Ah, I didn't notice that. Thanks for pointing it out. I'll wire it up and
> cross my fingers that the tests still pass.
Allow_skip is no longer present in the C API it is replaced with the
require_auth (now on the transport object) API.
So it would make more sense to implement that and remove allow_skip.
>
> --Rafael
Re: [38/38] qpid-proton git commit: implemented sasl sniffing for
proton-j; this allows the reactor interop tests to pass
Posted by Rafael Schloming <rh...@alum.mit.edu>.
On Mon, Jul 6, 2015 at 9:52 AM, Robbie Gemmell <ro...@gmail.com>
wrote:
> Is this change allowing clients to skip the SASL layer when connecting
> to servers that have enabled the SASL layer? If so, how is the new
> default behaviour disabled?
>
Yes, it was necessary to allow the tests to pass.
> The existing but unimplemented 'allowSkip' method previously intended
> to enable such behaviour still doesn't do anything, so is there a way
> to require clients use a SASL layer as would have been previously
> after enabling SASL for a proton-j (and in the past a proton-c)
> server?
>
Ah, I didn't notice that. Thanks for pointing it out. I'll wire it up and
cross my fingers that the tests still pass.
--Rafael