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

Improve and document support for encrypted keys #468

Merged
merged 5 commits into from
Feb 10, 2024

Conversation

PriceChild
Copy link
Contributor

Gives hints and limitations on encrypting the client key.

@github-actions github-actions bot added the Documentation Documentation needs updating label Feb 5, 2024
@PriceChild
Copy link
Contributor Author

PriceChild commented Feb 5, 2024

  • I haven't added the Signed-Off line yet. Will sort tomorrow.
  • Worried about whether this advice is safe, given discussions like this: https://security.stackexchange.com/a/203384/24342
  • Had a look at the ansible connection plugin, saw it'd likely need a change to the incus cli to accept client key before prompt?
  • While building docs, had to install a few other dependencies not mentioned on https://linuxcontainers.org/incus/docs/main/installing/#installing-from-source Might look at pr'ing that too.:
    • go 1.21 - jammy defaulted to 1.18
    • python3.10-venv for sphinx for make doc)
    • mdl (available from snap only?)
  • I couldn't find any better link for keepalive than the news page? Is there a reference for remote server config?
  • make doc-spellcheck has a couple of complaints...

@PriceChild PriceChild force-pushed the patch-1 branch 2 times, most recently from 9756265 to 61fe4f5 Compare February 6, 2024 17:11
@PriceChild
Copy link
Contributor Author

PriceChild commented Feb 6, 2024

@stgraber I'm missing something obvious with make doc-lint, running it locally gives:

$ make doc-lint
doc/.sphinx/.markdownlint/doc-lint.sh
Passed!

This didn't match the output of github actions?

I can run the actions in my own personal fork but that's obviously not ideal: https://github.com/PriceChild/incus/actions/runs/7803489771/job/21283375479

@stgraber stgraber force-pushed the patch-1 branch 4 times, most recently from 1788e5f to 0f987b2 Compare February 10, 2024 21:56
@stgraber
Copy link
Member

make doc-spellcheck has a couple of complaints...

Fixed those, it's the usual issue with capitalization of Incus and having a bunch of other stuff be escaped.

I couldn't find any better link for keepalive than the news page? Is there a reference for remote server config?

I've added a commit which adds a section for this now.

Had a look at the ansible connection plugin, saw it'd likely need a change to the incus cli to accept client key before prompt?

Yeah, that may need tweaking. You can work around it through the keepalive by first making a manual incus info remote: call to prompt for the key and start keepalive, then run Ansible before the timeout hits.

Worried about whether this advice is safe, given discussions like this: https://security.stackexchange.com/a/203384/24342

I've added support for SSH's encrypted key format, currently limited to ECDSA and update the documentation to recommend that for the EC case.

@stgraber stgraber marked this pull request as ready for review February 10, 2024 21:57
@stgraber stgraber self-requested a review as a code owner February 10, 2024 21:57
@stgraber
Copy link
Member

I've expanded support to also cover RSA keys through SSH encryption and so updated the instructions to always use ssh-keygen.

@stgraber stgraber changed the title doc: Hints on encrypting client key Improve and document support for encrypted keys Feb 10, 2024
@stgraber stgraber merged commit 4577279 into lxc:main Feb 10, 2024
25 checks passed
@PriceChild
Copy link
Contributor Author

Thank you for sorting that all out. I'm still not sure why the linting tools gave different results locally. Looking..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Documentation needs updating
Development

Successfully merging this pull request may close these issues.

2 participants