Added submodule Tide OAuth. #172

sonnykt · 2020-09-03T06:04:47Z

On-behalf-of: @salsadigitalauorg sonny@salsadigital.com.au

On-behalf-of: @salsadigitalauorg <sonny@salsadigital.com.au>

anthony-malkoun · 2020-10-27T05:08:03Z

@sonnykt for my understanding why would we ever want to provide env variables for generating keys? Is it good enough to let them be generated every time? The main motivation here is that the AUTHENTICATED_CONTENT key has been painful to maintain (Lagoon does not handle multiline config variables correctly) and meant that it had to be added manually for PR environments (and remember to add it for new environments). If there is value to having a known value for the keys, can it be something else like the token that TFA uses or does it have to be a full RSA key?

anthony-malkoun · 2020-10-27T05:09:41Z

modules/tide_oauth/README.md

+    * If the TIDE_OAUTH_ environment variables are set, the module will copy
+    the keys to `private://oauth.key` and `private://oauth.pub`.
+    * Otherwise, the module will generate a new key pair.
+3. _(Optional - **Lagoon only**)_ Add a `post-rollout` task to generate the OAuth


Is there any reason this couldn't be an openssl command to generate on the fly from scratch?

if the key changes, all existing tokens become invalid, even if they are not expired.

So if the deploy step checked if the key existed first before trying to create it, then it would work?

@anthony-malkoun yes https://github.com/dpc-sdp/tide_core/pull/172/files#diff-6f2392828bd5ca4ff53608718b20927df9c8d6d10a19951924aa36803885473dR51

it only happens upon installation. If it's already on Prod and you launch a new dev env, the key pair doesn't exist on the new env and it still fails. There is no mechanism for Drupal to check the key pair upon init, hence the lagoon post-rollout task to trigger the Drush command copy the env to the key pair. The Drush command doesn't generate a new key pair if the env var doesn't exist.

Do you see any issue if we use a HMAC key rather than an RSA key @sonnykt ? It should just be the instructions that change. HMAC will be a single line token that can then be defined at the project level in Lagoon (and overridden for production) and will be able to be managed by our configuration management.

anthony-malkoun · 2020-10-27T05:11:00Z

modules/tide_oauth/composer.json

@@ -0,0 +1,12 @@
+{


We are still on Drush 8, does this present an issue here?

it's not an issue. drush.services.yml only takes effect from Drush 9. That snippet ensures that we support both 8 and 9.

sonnykt · 2020-10-27T06:47:07Z

@sonnykt for my understanding why would we ever want to provide env variables for generating keys? Is it good enough to let them be generated every time? The main motivation here is that the AUTHENTICATED_CONTENT key has been painful to maintain (Lagoon does not handle multiline config variables correctly) and meant that it had to be added manually for PR environments (and remember to add it for new environments). If there is value to having a known value for the keys, can it be something else like the token that TFA uses or does it have to be a full RSA key?

If there is no env variable, the module will generate a random key pair upon its installation. The env var is to ensure that all envs have the same key pair.

anthony-malkoun · 2020-10-27T08:05:36Z

@sonnykt for my understanding why would we ever want to provide env variables for generating keys? Is it good enough to let them be generated every time? The main motivation here is that the AUTHENTICATED_CONTENT key has been painful to maintain (Lagoon does not handle multiline config variables correctly) and meant that it had to be added manually for PR environments (and remember to add it for new environments). If there is value to having a known value for the keys, can it be something else like the token that TFA uses or does it have to be a full RSA key?

If there is no env variable, the module will generate a random key pair upon its installation. The env var is to ensure that all envs have the same key pair.

And more importantly, if I understand correctly @sonnykt that they remain the same after deploy? See my previous comment re: using a HMAC rather than RSA. If this is ok then I'm ok with this. Maintaining the AUTHENTICATED_CONTENT key has been a complete pain which I want to avoid.

sonnykt · 2020-10-27T11:44:04Z

@sonnykt for my understanding why would we ever want to provide env variables for generating keys? Is it good enough to let them be generated every time? The main motivation here is that the AUTHENTICATED_CONTENT key has been painful to maintain (Lagoon does not handle multiline config variables correctly) and meant that it had to be added manually for PR environments (and remember to add it for new environments). If there is value to having a known value for the keys, can it be something else like the token that TFA uses or does it have to be a full RSA key?

If there is no env variable, the module will generate a random key pair upon its installation. The env var is to ensure that all envs have the same key pair.

And more importantly, if I understand correctly @sonnykt that they remain the same after deploy? See my previous comment re: using a HMAC rather than RSA. If this is ok then I'm ok with this. Maintaining the AUTHENTICATED_CONTENT key has been a complete pain which I want to avoid.

I don't think OAuth2 server accepts HMAC key. It requires a key pair.

anthony-malkoun · 2020-10-28T03:22:04Z

Thinking some more about this @sonnykt and the install creates a valid key in public:// which will be persisted across builds. Do you see any issue with just using this initial key ongoing? Because of short comings of environment variables in Lagoon I don't think we will ever need to generate them from environment variables. Thoughts?

sonnykt · 2020-10-28T04:00:28Z

Thinking some more about this @sonnykt and the install creates a valid key in public:// which will be persisted across builds. Do you see any issue with just using this initial key ongoing? Because of short comings of environment variables in Lagoon I don't think we will ever need to generate them from environment variables. Thoughts?

The module installation creates a key pair in private:// (not public) and the key pair remains in the persistent storage. However, once the module is enabled on production and you later launch a new non-prod env, the module does not create a new key pair on that env because it is already installed in the database. The new env doesn't have a valid key pair. The key pair from prod is not copied to the new env, hence the module fails to work.

The post-rollout task runs the Drush command to generate the key pair from the env variables, but it doesn't generate a new key pair if the env variables are not set. We could modify that command to do:

check if there is a valid key pair in private://
if not, try to create a key pair from env variables
if the env variables are not set, generate a new key pair.

anthony-malkoun · 2020-10-28T05:08:35Z

Thinking some more about this @sonnykt and the install creates a valid key in public:// which will be persisted across builds. Do you see any issue with just using this initial key ongoing? Because of short comings of environment variables in Lagoon I don't think we will ever need to generate them from environment variables. Thoughts?

The module installation creates a key pair in private:// (not public) and the key pair remains in the persistent storage. However, once the module is enabled on production and you later launch a new non-prod env, the module does not create a new key pair on that env because it is already installed in the database. The new env doesn't have a valid key pair. The key pair from prod is not copied to the new env, hence the module fails to work.

The post-rollout task runs the Drush command to generate the key pair from the env variables, but it doesn't generate a new key pair if the env variables are not set. We could modify that command to do:

check if there is a valid key pair in private://

if not, try to create a key pair from env variables

if the env variables are not set, generate a new key pair.

I think this is the best solution. That post deploy step can stay in place as it won't be destructive. Do you think it needs some sort of --refresh or --force flag that can get it to generate new key pair, ignoring what's in private://.
It would be nice if Simple OAuth supported the key module (unfortunately not https://www.drupal.org/project/simple_oauth/issues/3133698 - the issue being a short coming in the implementation of the library beneath the module thephpleague/oauth2-server#1007) but that's not an option.

On-behalf-of: @salsadigitalauorg <sonny@salsadigital.com.au>

…ide_oauth

anthony-malkoun · 2020-12-04T05:00:20Z

The post-rollout task runs the Drush command to generate the key pair from the env variables, but it doesn't generate a new key pair if the env variables are not set. We could modify that command to do:

check if there is a valid key pair in private://

if not, try to create a key pair from env variables

if the env variables are not set, generate a new key pair.

Hey @sonnykt did this get done?

sonnykt · 2020-12-06T23:42:08Z

The post-rollout task runs the Drush command to generate the key pair from the env variables, but it doesn't generate a new key pair if the env variables are not set. We could modify that command to do:

check if there is a valid key pair in private://

if not, try to create a key pair from env variables

if the env variables are not set, generate a new key pair.

Hey @sonnykt did this get done?

@anthony-malkoun I doubt that. IIRC we only discussed but haven't got a final decision yet.

anthony-malkoun · 2020-12-07T02:48:12Z

The post-rollout task runs the Drush command to generate the key pair from the env variables, but it doesn't generate a new key pair if the env variables are not set. We could modify that command to do:

check if there is a valid key pair in private://

if not, try to create a key pair from env variables

if the env variables are not set, generate a new key pair.

Hey @sonnykt did this get done?

@anthony-malkoun I doubt that. IIRC we only discussed but haven't got a final decision yet.

Who are you waiting for a final decision from? I'm happy for this to happen.

sonnykt · 2020-12-07T03:03:14Z

@anthony-malkoun you'd probably get @vincent-gao to make the change as my allocation this week is for VT.

…keys. (#197) * [tide_oauth_changes] Updated drush command to generate keys if no envkeys.

Added submodule Tide OAuth.

5794f02

On-behalf-of: @salsadigitalauorg <sonny@salsadigital.com.au>

sonnykt self-assigned this Sep 3, 2020

sonnykt added DO NOT MERGE DO NOT REVIEW labels Sep 3, 2020

sonnykt marked this pull request as ready for review September 3, 2020 06:25

Made sure private:// is available before generating keys.

0dc0669

On-behalf-of: @salsadigitalauorg <sonny@salsadigital.com.au>

sonnykt force-pushed the feature/tide_oauth branch 2 times, most recently from fe71cbe to ccf34e4 Compare September 4, 2020 03:07

Allowed Authorization: OAuth2 header.

84fcbb3

On-behalf-of: @salsadigitalauorg <sonny@salsadigital.com.au>

sonnykt force-pushed the feature/tide_oauth branch from ccf34e4 to 84fcbb3 Compare September 4, 2020 03:19

sonnykt requested a review from anthony-malkoun September 4, 2020 04:07

sonnykt added Needs review and removed DO NOT MERGE DO NOT REVIEW labels Sep 4, 2020

sonnykt and others added 4 commits October 2, 2020 11:45

Set correct chmod for generated keys upon installation.

b449b1d

On-behalf-of: @salsadigitalauorg <sonny@salsadigital.com.au>

Added Simple Oauth2 patch for issue #2958159.

f6b4f4b

On-behalf-of: @salsadigitalauorg <sonny@salsadigital.com.au>

Merge branch 'develop' into feature/tide_oauth

51b519a

Merge branch 'develop' into feature/tide_oauth

e0e14ca

anthony-malkoun reviewed Oct 27, 2020

View reviewed changes

sonnykt added 2 commits November 24, 2020 16:45

Added custom help text to Editorial Preview consumer.

cd0452c

On-behalf-of: @salsadigitalauorg <sonny@salsadigital.com.au>

Merge branch 'develop' of github.com:dpc-sdp/tide_core into feature/t…

20ab5d0

…ide_oauth

vincent-gao force-pushed the feature/tide_oauth branch from dac7a1c to 20ab5d0 Compare December 3, 2020 01:16

vincent-gao added 2 commits December 3, 2020 16:06

add a field and change consumer’s label

09f026b

lint

409ff81

vincent-gao requested a review from anthony-malkoun December 3, 2020 23:52

Merge branch 'develop' into feature/tide_oauth

dd1eba8

[tide_oauth_changes] Updated drush command to generate keys if no env…

82009df

…keys. (#197) * [tide_oauth_changes] Updated drush command to generate keys if no envkeys.

anthony-malkoun approved these changes Dec 8, 2020

View reviewed changes

MdNadimHossain merged commit e091b57 into develop Dec 8, 2020

MdNadimHossain deleted the feature/tide_oauth branch December 8, 2020 04:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added submodule Tide OAuth. #172

Added submodule Tide OAuth. #172

sonnykt commented Sep 3, 2020

anthony-malkoun commented Oct 27, 2020

anthony-malkoun Oct 27, 2020

sonnykt Oct 27, 2020

anthony-malkoun Oct 27, 2020

sonnykt Oct 27, 2020

sonnykt Oct 27, 2020 •

edited

anthony-malkoun Oct 27, 2020

anthony-malkoun Oct 27, 2020

sonnykt Oct 27, 2020

anthony-malkoun Oct 27, 2020

sonnykt commented Oct 27, 2020

anthony-malkoun commented Oct 27, 2020

sonnykt commented Oct 27, 2020

anthony-malkoun commented Oct 28, 2020

sonnykt commented Oct 28, 2020

anthony-malkoun commented Oct 28, 2020

anthony-malkoun commented Dec 4, 2020

sonnykt commented Dec 6, 2020 •

edited

anthony-malkoun commented Dec 7, 2020

sonnykt commented Dec 7, 2020

Added submodule Tide OAuth. #172

Added submodule Tide OAuth. #172

Conversation

sonnykt commented Sep 3, 2020

anthony-malkoun commented Oct 27, 2020

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

sonnykt Oct 27, 2020 • edited

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

sonnykt commented Oct 27, 2020

anthony-malkoun commented Oct 27, 2020

sonnykt commented Oct 27, 2020

anthony-malkoun commented Oct 28, 2020

sonnykt commented Oct 28, 2020

anthony-malkoun commented Oct 28, 2020

anthony-malkoun commented Dec 4, 2020

sonnykt commented Dec 6, 2020 • edited

anthony-malkoun commented Dec 7, 2020

sonnykt commented Dec 7, 2020

sonnykt Oct 27, 2020 •

edited

sonnykt commented Dec 6, 2020 •

edited