OAuth client side state #459

zregvart · 2017-07-25T10:32:12Z

This is part of the work for #457, this can be viewed independently, so I'm creating a PR for early review/merge.

jimmibot · 2017-07-25T10:32:14Z

@zregvart, thanks! @iocanel, @jimmidyson and @rhuss, please review this.

jimmidyson · 2017-07-25T13:59:27Z

rest/src/main/java/io/syndesis/rest/v1/state/ClientSideState.java

+    private final Supplier<Long> timeSource;
+
+    protected static final class RandomIvSource implements Supplier<byte[]> {
+        private static final ThreadLocal<SecureRandom> RANDOM = new ThreadLocal<SecureRandom>() {


Isn't SecureRandom threadsafe? Do we really want to initialize (expensive) one for every thread we use this on?

Yeah it is, this pattern is something I usually do, but now I remembered that we run in Linux containers so it will go to /dev/urandom, it would block otherwise on Windows and that makes it needed. Good catch 👍

jimmidyson · 2017-07-25T14:00:15Z

rest/src/main/java/io/syndesis/rest/v1/state/ClientSideState.java

+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.ObjectWriter;
+
+import org.msgpack.jackson.dataformat.MessagePackFactory;


How much do we save using MessagePack vs JSON?

i.e. is it worth introducing a new dependency for this?

Looking at empty OAuth2CredentialFlowState (current working, ex CredentialFlowState) it's 30% difference (80 bytes JSON, 56 bytes MessagePack). We have 2.8KB of space in the Cookie to work with. If it's an issue easily removed 👂

I'm not fussed either way to be honest, msgpack format would bring in an extra 2 dependencies but that's no biggie. So I'll leave it up to you :)

Yeah, let's use it when we see it's needed.

BTW we have also think about productisation. As I've been told, every single dependency needs to be productised (i.e. needs to be build by red hat). See https://issues.jboss.org/browse/ENTESB-6963 for details and the dependency list we already have.

jimmidyson · 2017-07-25T14:02:50Z

runtime/src/main/resources/application.yml

+
+client:
+  state:
+    authenticationAlgorithm: HmacSHA1


Did you mean to add these as default settings? How about leaving these out of here?

I'm don't fully understand, you mean have them as default so that they don't need to be specified or remove them entirely?

Sorry what I mean was, do we need this configuration in the application.yml that is bundled with the app at build time? Surely this is runtime configuration only and as such should be provided by the environment (via the syndesis-rest configmap in this case)? I don't like having stuff like encryption keys as defaults as it increases the chance of a publicly readable encryption key accidentally being used in a real deployment, e.g. if the configmap doesn't have the right configuration.

So I mean remove them entirely.

Yeah, that's what I was planing on doing, but in a separate PR as this needs some thinking behind it. Right now you can override these via environment variables. Not sure if we can have them dynamically set in the OpenShift templates.

Things to think about:

do we need this statically or dynamically (think regenerate keys every n days)

do we do this via k8s secrets

if dynamically how to handle the transition (need n-1 keys)

if dynamically then we need key distribution (spring cloud k8s, but it reloads the app context)

I still would like these removed from the committed codebase due to the potential problems I mentioned above.

We could prevent startup if these values aren't set perhaps so we know they have to be set in prod deployments?

Keys should be regenerated, short lived keys are great, but you're right we need to think about how we do that. I'd recommend we do a rolling deployment rather than updating in place, but we also need to figure out how we accept old encrypted cookies and re-encrypt on next request if we do that. But this can be future PR work IMO.

zregvart · 2017-07-26T10:14:00Z

I'd like to do more manual testing before I declare this done, but it should not change much so reviews are welcome 👍

zregvart · 2017-07-26T14:13:51Z

Manually tested with Salesforce, works 👍

jimmidyson

Mostly looks really good, some refactoring changes that I think will help keep oauth1/2 flow logic more separate and easier to follow, but overall 👍. Thanks again @zregvart!

P.S. I know some of the requested changes are not changes necessarily made in this PR, but they relate to the same area of changes, i.e. making oauth1/2 flows more self contained and easier to follow. If it's going to significantly delay delivery or if the changes actually don't make sense, please let us know ASAP and we can look again in future.

jimmidyson · 2017-07-26T16:34:11Z

credential/src/main/java/io/syndesis/credential/CredentialFlowState.java

+    }
+
+    default String statePrefix() {
+        if (this instanceof OAuth1CredentialFlowState) {


How about moving these down into the specific interfaces?

yeah, silly me, good catch 👍

jimmidyson · 2017-07-26T16:34:22Z

credential/src/main/java/io/syndesis/credential/CredentialFlowState.java

+    }
+
+    default Type type() {
+        if (this instanceof OAuth1CredentialFlowState) {


Same here: How about moving these down into the specific interfaces?

jimmidyson · 2017-07-26T16:34:31Z

credential/src/main/java/io/syndesis/credential/CredentialFlowState.java

+    CredentialFlowState updateFrom(HttpServletRequest request);
+
+    static Builder builderFor(final ConnectionFactory<?> connectionFactory) {
+        if (connectionFactory instanceof OAuth1ConnectionFactory) {


And here... How about moving these down into the specific interfaces?

jimmidyson · 2017-07-26T16:36:05Z

credential/src/main/java/io/syndesis/credential/CredentialFlowState.java

+        throw new IllegalStateException("Unsupported connection factory implementation: " + connectionFactory);
+    }
+
+    static Class<? extends CredentialFlowState> typeForName(final String name) {


Why is this necessary? Isn't jackson smart enough to do (de)serialize appropriately for subtypes?

I think I need to give Jackson a concrete type, but I did fiddle with the annotations so it could be leftover from when id did not work for me. I think I need a unit test for that.

jimmidyson · 2017-07-26T16:38:02Z

credential/src/main/java/io/syndesis/credential/Credentials.java


        if (connectionFactory instanceof OAuth1ConnectionFactory) {
-            redirectUrl = prepareOAuth1((OAuth1ConnectionFactory<?>) connectionFactory, flowStateBuilder, request);
+            redirectUrl = prepareOAuth1((OAuth1ConnectionFactory<?>) connectionFactory,


Why not move all these prepareOAuth{1,2} and similar methods into relevant connection factories?

Good point, will try 👍

jimmidyson · 2017-07-26T16:39:59Z

pom.xml

@@ -543,6 +543,18 @@
        <version>3.2.1</version>
      </dependency>

+      <dependency>
+        <groupId>org.msgpack</groupId>


So you decided to keep these? That's cool, just misunderstood what you said before, but no worries.

Oh, no, they're out, you know how I like to force push commits 😈

Yeah, forgot to remove it from the parent

zregvart · 2017-07-27T09:37:34Z

I think its well worth the effort to address the comments from the review in this PR, it should not be a lot of work.

zregvart · 2017-07-27T13:23:46Z

So with the latest commit there is a definite split between OAuth1 and OAuth2 implementations and this I think improved greatly the simplicity of the implementation.

@jimmidyson could you have another look?

zregvart · 2017-07-28T08:27:45Z

I think the build is failing because deployment descriptor is missing environment variables trying to catch pod failing in system tests to confirm.

See syndesisio/syndesis-openshift-templates#63

zregvart · 2017-07-28T09:01:30Z

Indeed:

org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'credentialHandler' defined in URL [jar:file:/deployments/runtime.jar!/BOOT-INF/lib/rest-0.1-SNAPSHOT.jar!/io/syndesis/rest/v1/handler/credential/CredentialHandler.class]: Unsatisfied dependency expressed through constructor parameter 1; nested exception is org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'clientSideState' defined in io.syndesis.runtime.Application: Bean instantiation via factory method failed; nested exception is org.springframework.beans.BeanInstantiationException: Failed to instantiate [io.syndesis.rest.v1.state.ClientSideState]: Factory method 'clientSideState' threw exception; nested exception is org.springframework.beans.factory.BeanCreationException: Client side state persistence configuration is not valid, please set
    CLIENT_STATE_AUTHENTICATION_ALGORITHM
    CLIENT_STATE_AUTHENTICATION_KEY
    CLIENT_STATE_ENCRYPTION_ALGORITHM
    CLIENT_STATE_ENCRYPTION_KEY
    CLIENT_STATE_TID
environment variables.

jimmidyson

Minor nit, one question.

Much easier to follow - thanks again @zregvart!

jimmidyson · 2017-07-28T09:18:14Z

credential/src/main/java/io/syndesis/credential/OAuth1CredentialFlowState.java

+
+    }
+
+    final class OAuthTokeConverter implements Converter<Map<String, String>, OAuthToken> {


nit: typo Toke -> Token

jimmidyson · 2017-07-28T09:25:17Z

rest/src/main/java/io/syndesis/rest/v1/state/Edition.java

+/**
+ * One configuration edition of settings used to store state on the client.
+ */
+public abstract class Edition {


Bit confused by the name? Could you explain why it's called Edition?

Yeah I was trying to find a good name, the idea always was to periodically regenerate the keys, then each new generation would have tid=tid+1, so as they roll in time I thought Edition would be a good name for it.

jimmidyson

OK LGTM. Please squash and let's wait until CI is passing.

zregvart · 2017-07-28T11:27:10Z

squashing...

Comments addressed

...nt side state OAuth1 and OAuth2 implementations used different state storage, this changes that to use the same storage, which has been implemented on the client side as a cookie. For this the flow state had to be split into two variants one for OAuth1 and one for OAuth2, and state access had to be revised thought the implementation. This implements persisting state on the client side via secure cookies as defined in RFC6896[1]. This is basis for storing OAuth flow state in syndesisio#457. The configuration needed for this to function can be specified via environment variables: CLIENT_STATE_AUTHENTICATION_ALGORITHM _(e.g. HmacSHA1)_ CLIENT_STATE_AUTHENTICATION_KEY _(e.g. random 20 bytes)_ CLIENT_STATE_ENCRYPTION_ALGORITHM _(e.g. AES/CBC/PKCS5Padding)_ CLIENT_STATE_ENCRYPTION_KEY _(e.g. random 20 bytes)_ CLIENT_STATE_TID _(e.g. 1)_ If configuration is not defined random values are generated, wich will issue a warning as random values in pod instances will not failover or work after restart as the keys would differ. Also, separated OAuth1 and OAuth2 logic, it is now pushed in the `CredentialProvider`s so no more iffy logic in `Credentials`, yay! [1] https://tools.ietf.org/html/rfc6896 Fixes syndesisio#457

pure-bot · 2017-07-28T13:03:03Z

Pull request approved by @jimmidyson - applying approved label

jimmibot assigned iocanel, jimmidyson and rhuss Jul 25, 2017

jimmibot requested review from iocanel, jimmidyson and rhuss July 25, 2017 10:32

zregvart force-pushed the #457 branch from af9ef25 to 3e70866 Compare July 25, 2017 13:38

jimmidyson reviewed Jul 25, 2017

View reviewed changes

zregvart force-pushed the #457 branch from 3e70866 to f21f3c1 Compare July 25, 2017 14:16

zregvart mentioned this pull request Jul 26, 2017

Don't use hard coded keys for client side state #460

Closed

zregvart force-pushed the #457 branch from d196958 to 51c7949 Compare July 26, 2017 10:06

zregvart force-pushed the #457 branch 2 times, most recently from 2e8740c to 125f6c3 Compare July 26, 2017 14:33

syndesisio deleted a comment Jul 26, 2017

jimmidyson suggested changes Jul 26, 2017

View reviewed changes

zregvart force-pushed the #457 branch from f230bc4 to 5691385 Compare July 27, 2017 13:22

jimmidyson previously requested changes Jul 28, 2017

View reviewed changes

jimmidyson reviewed Jul 28, 2017

View reviewed changes

zregvart force-pushed the #457 branch from f644afe to bc9ce82 Compare July 28, 2017 11:31

jimmidyson approved these changes Jul 28, 2017

View reviewed changes

pure-bot bot added the approved label Jul 28, 2017

pure-bot bot merged commit d3d61dd into syndesisio:master Jul 28, 2017

zregvart deleted the #457 branch July 28, 2017 13:03

syndesis-bot mentioned this pull request Nov 15, 2017

Don't use hard coded keys for client side state syndesisio/syndesis#58

Closed


		}

		final class OAuthTokeConverter implements Converter<Map<String, String>, OAuthToken> {

OAuth client side state #459

OAuth client side state #459

Conversation

zregvart commented Jul 25, 2017

jimmibot commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zregvart commented Jul 26, 2017

zregvart commented Jul 26, 2017

jimmidyson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zregvart commented Jul 27, 2017

zregvart commented Jul 27, 2017

zregvart commented Jul 28, 2017

zregvart commented Jul 28, 2017

jimmidyson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmidyson left a comment

Choose a reason for hiding this comment

zregvart commented Jul 28, 2017

pure-bot bot commented Jul 28, 2017