Skip to content
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

Use signed cookies in integration tests [RHELDST-8860] #323

Merged
merged 1 commit into from
Mar 8, 2022

Conversation

dichn
Copy link
Collaborator

@dichn dichn commented Mar 1, 2022

If tests are run without the two env vars "EXODUS_CDN_PRIVATE_KEY"
and "EXODUS_CDN_PRIVATE_KEY", requests will not be signed.

@dichn dichn marked this pull request as draft March 1, 2022 11:01
@dichn dichn force-pushed the RHELDST-8860 branch 4 times, most recently from 0d003c0 to 782ac0b Compare March 1, 2022 11:25
@dichn dichn marked this pull request as ready for review March 1, 2022 14:05
@dichn
Copy link
Collaborator Author

dichn commented Mar 1, 2022

@negillett @crungehottman @rohanpm
I think this PR can only be merged until the corresponding ansible playbook supports deploying signing keys. Then, the integration tests can pass (I tested it on my CloudFront distribution which configured the signing keys manually in AWS web console).
This patch handles:
(1) Read the private-key and key-pair-id from env var
(2) Generate signed cookies
(3) Send requests to CloudFront with the signed cookies

@negillett
Copy link
Member

negillett commented Mar 1, 2022

@negillett @crungehottman @rohanpm I think this PR can only be merged until the corresponding ansible playbook supports deploying signing keys. Then, the integration tests can pass (I tested it on my CloudFront distribution which configured the signing keys manually in AWS web console). This patch handles: (1) Read the private-key and key-pair-id from env var (2) Generate signed cookies (3) Send requests to CloudFront with the signed cookies

Yeah, I think a lot of this depends on RHELDST-8859. And I nearly have it ready for code review. #324

tests/test_utils/utils.py Outdated Show resolved Hide resolved
tests/test_utils/utils.py Outdated Show resolved Hide resolved
Comment on lines 90 to 89

def generate_cookies():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've implemented signing code in exodus_lambda/functions/signer.py in #324

Copy link
Member

@negillett negillett Mar 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That PR may enable deployment of testing key(s) for the integration tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@negillett
if using the exodus_lambda/functions/signer.py and its signer.cookies_for_policy().

        cookies_content = signer.cookies_for_policy(
            append="; Secure; Path=/content/; Max-Age=%s"
            % int(expiration.total_seconds()),
            resource="https://%s/content/*" % domain,

I think it requires integration tests ask for cookies for every request (r = requests.get(url, cookies=TEST_COOKIES))
The AC for integration tests is one cookies for all requests, so maybe not using the new cookies endpoint in #324, WDYT?
CC: @crungehottman @rohanpm

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it wasn't the intent of this issue to use the new cookies endpoint for all the tests. (There could be one or two integration tests explicitly targeted at that endpoint though).

In fact, you can't directly use it anyway. Remember the new cookies endpoint is itself protected - you must have a signed URL or cookies in order to get there. So, for exodus-gw => cookies endpoint => any content, that's OK as exodus-gw can generate signed URLs. For the tests, in order to use the cookies endpoint they'd have to replace exodus-gw in that process, meaning there would be two layers of signing unnecessarily. (The only reason we have two layers in the exodus-gw case is that one domain isn't allowed to set cookies for another domain.)

Copy link
Member

@negillett negillett Mar 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dichn,
My point is only that some of the stuff from #324 can be reused here.
I.e., you could adapt the generate_cookies function. Something like this should work;

def generate_test_cookies():
    key = os.environ.get("EXODUS_CDN_PRIVATE_KEY")
    key_id = os.environ.get("EXODUS_CDN_KEY_ID")

    if not key_id or not key:
        return {}

    signer = Signer(key, key_id)
    expiration = datetime.utcnow() + timedelta(minutes=60)
    return signer.cookies_for_policy(append="", date_less_than=expiration)

tests/test_utils/utils.py Outdated Show resolved Hide resolved

def generate_cookies():
# Env var for using signed cookies
CDN_PRIVATE_KEY = os.environ.get("EXODUS_CDN_PRIVATE_KEY")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: local vars shouldn't be named in all caps. You'd name these CDN_PRIVATE_KEY etc if they were globals, which they aren't.

@dichn dichn self-assigned this Mar 2, 2022
@dichn dichn marked this pull request as draft March 3, 2022 01:41
@dichn dichn force-pushed the RHELDST-8860 branch 8 times, most recently from 7b1cf06 to 79e8616 Compare March 7, 2022 07:35
@dichn
Copy link
Collaborator Author

dichn commented Mar 7, 2022

Status:

  • move CloudFrontSigner::build_policy() out of Signer::cookies_for_policy, so tests can do some customization to the policy (remove the "resource" parameter, so TEST_COOKIES can access all the test content)
  • fixed the typo in env vars reading
    CC: @rohanpm @crungehottman @negillett

@dichn dichn marked this pull request as ready for review March 7, 2022 07:50
@dichn dichn requested review from rohanpm and negillett March 7, 2022 07:52
@dichn dichn force-pushed the RHELDST-8860 branch 2 times, most recently from a99a207 to edc06d0 Compare March 7, 2022 08:37
@negillett
Copy link
Member

negillett commented Mar 7, 2022

@dichn

  • move CloudFrontSigner::build_policy() out of Signer::cookies_for_policy, so tests can do some customization to the policy (remove the "resource" parameter, so TEST_COOKIES can access all the test content)

This works but seems unnecessary. Instead of doing all that to omit/remove resource from the policy, you just needed to add one accepted by all test requests, e.g., "https://*".
Applying that to my earlier suggestion:

def generate_test_cookies():
    key = os.environ.get("EXODUS_CDN_PRIVATE_KEY")
    key_id = os.environ.get("EXODUS_CDN_KEY_ID")

    if not key_id or not key:
        return {}

    signer = Signer(key, key_id)
    expiration = datetime.utcnow() + timedelta(minutes=60)
    return signer.cookies_for_policy(append="", resource="https://*", date_less_than=expiration)

@dichn
Copy link
Collaborator Author

dichn commented Mar 8, 2022

@negillett many thanks!!! It works and much simpler than the previous change.

rohanpm
rohanpm previously approved these changes Mar 8, 2022
# envvar absent, requests will not be signed
return {}

# The cookies expire in 1 hour
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: it's a bit funny to write "expire in 1 hour", then "timedelta(seconds=3600)" when you could just write "timedelta(hours=1)". The main point of that class seems to be it allows you to deal with units other than seconds if you want.

If tests are run without the two env vars "EXODUS_CDN_PRIVATE_KEY"
and "EXODUS_CDN_KEY_ID", requests will not be signed.
@dichn dichn merged commit 221649f into release-engineering:master Mar 8, 2022
@dichn dichn deleted the RHELDST-8860 branch March 8, 2022 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants