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

Generate authentication keys #3478

Merged
merged 3 commits into from
Jan 28, 2020
Merged

Generate authentication keys #3478

merged 3 commits into from
Jan 28, 2020

Conversation

datamel
Copy link
Contributor

@datamel datamel commented Jan 12, 2020

I suspect this PR will result in further changes to code so I will squash once fully ready to be merged.

Keys are now all stored in the .service directory of the suite rather than the client keys atored in the .cylc directory.

Keys are no longer regenerated - cylc errors are raised instead

This has been left extensible for future multiple platform key generation.

Closes #3444
Closes #3445

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included (unit and/or functional).
  • No change log entry required - invisible to users.
  • Documentation PR to be competed authentication: wash up #3449

@datamel datamel self-assigned this Jan 12, 2020
@datamel datamel added the security security-related issues label Jan 12, 2020
@datamel datamel added this to the cylc-8.0a2 milestone Jan 12, 2020
Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Not really a proper review. Just quickly read it to know more or less what's changing. But great improvements with f-strings, and also good unit tests! 👏

cylc/flow/suite_files.py Outdated Show resolved Hide resolved
cylc/flow/suite_files.py Outdated Show resolved Hide resolved
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Looks good, a few small comments which should be quick fixes. Namely we need to swap the order in which we set permissions and move files to make sure they don't become vulnerable in between.

cylc/flow/suite_files.py Outdated Show resolved Hide resolved
cylc/flow/suite_files.py Outdated Show resolved Hide resolved
cylc/flow/suite_files.py Outdated Show resolved Hide resolved
cylc/flow/suite_files.py Show resolved Hide resolved
cylc/flow/suite_files.py Outdated Show resolved Hide resolved
cylc/flow/suite_files.py Outdated Show resolved Hide resolved
cylc/flow/suite_files.py Outdated Show resolved Hide resolved
cylc/flow/suite_files.py Outdated Show resolved Hide resolved
cylc/flow/suite_files.py Outdated Show resolved Hide resolved
cylc/flow/tests/network/test_zmq.py Outdated Show resolved Hide resolved
@hjoliver
Copy link
Member

(@dpmatthews plans to take a close look, and do some testing, before others pitch in here)

@datamel
Copy link
Contributor Author

datamel commented Jan 22, 2020

@dpmatthews and I have just been through this together. I think it is now ready for review, @hjoliver I will add you to the list of reviewers. Thank you to all for the comments so far.
Note that this PR will also close #3444 in addition to #3445.
I think it is all now working as expected and remote authentication is now working too.
There is a separate issue I will raise to ensure errors are recorded correctly when keys are missing remotely.
The authentication keys are now all stored in: <SUITE_NAME>/.service. Client public keys have their own sub directory: <SUITE_NAME>/.service/client_public_keys.
There is still much legacy code to remove. This will be removed in ticket: authentication: wash up #3449.
I will also rebase/squash once everyone is happy and it is ready for merge.

@datamel datamel requested a review from hjoliver January 22, 2020 15:57
Copy link
Member

@dwsutherland dwsutherland left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Works end-to-end WFS <=> UIS <=> UI

2020-01-24T17:01:08+13:00 DEBUG - Starting
2020-01-24T17:01:08+13:00 DEBUG - auth received API command b'CURVE'
2020-01-24T17:01:08+13:00 DEBUG - Configure curve: *[/home/sutherlander/cylc-run/baz/.service/client_public_keys]
.
.
.
2020-01-24T17:01:47+13:00 DEBUG - version: b'1.0', request_id: b'1', domain: '', address: '127.0.0.1', identity: b'', mechanism: b'CURVE'
2020-01-24T17:01:47+13:00 DEBUG - ALLOWED (CURVE) domain=* client_key=b'@:LiHf*406UmxVmgT1dyY.tZ3gN3:XhZxXv?/i{Q'
2020-01-24T17:01:47+13:00 DEBUG - ZAP reply code=b'200' text=b'OK'
2020-01-24T17:02:11+13:00 DEBUG - version: b'1.0', request_id: b'1', domain: '', address: '127.0.0.1', identity: b'', mechanism: b'CURVE'
2020-01-24T17:02:11+13:00 DEBUG - ALLOWED (CURVE) domain=* client_key=b'@:LiHf*406UmxVmgT1dyY.tZ3gN3:XhZxXv?/i{Q'
2020-01-24T17:02:11+13:00 DEBUG - ZAP reply code=b'200' text=b'OK'
2020-01-24T17:02:11+13:00 INFO - [client-command] release_suite sutherlander@cortex-vbox:cylc-release
.
.
.

Definitely some improvements ... Will have another read and try to think about it in the wider context.

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Code looks good.

Tested as working including remote functionality 🚀

Cylc creates the two keys we expect it to; client (cli) and server (ser) both in public (pub) and private (pri) variants:

  • @localhost ~/cylc-run/suite/.service/
    • cli-pri client.key_secret
    • cli-pub client_public_keys/client.key
    • ser-pri server.key_secret
    • ser-pub server.key

On the remote hosts Cylc copies two keys across:

  • @remotehost ~/cylc-run/<suite>/.service/
    • cli-pri client.key_secret
    • ser-pub server.key

(I find the filenames curve/zmq uses for these keys really darned confusing).

Note: Copying the private client key is bad security but this is set aside for future work #3443

@oliver-sanders
Copy link
Member

A quick rebase and we are good to go!

Copy link
Contributor

@dpmatthews dpmatthews left a comment

Choose a reason for hiding this comment

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

Remote tests now working fine for me

- Change file permissions for public/private keys
- Stop keys being regenerated

- Tests added.

Remove passphrase bits breaking functional tests
Replaced **kwargs from suite_files keyinfo object init

Fixed suite names in tests.
@datamel datamel merged commit 26f5b43 into cylc:master Jan 28, 2020
@datamel datamel deleted the generate_certs branch September 20, 2022 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security security-related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

authentication: separate client keys per suite authentication: remote functionality
6 participants