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 ca experience #157

Merged
merged 14 commits into from
Apr 9, 2023
Merged

Improve ca experience #157

merged 14 commits into from
Apr 9, 2023

Conversation

hannesm
Copy link
Collaborator

@hannesm hannesm commented Mar 1, 2023

with this PR, unikernel creation and policy addition are checked before a connection is established / a certificate is generated

addresses some parts of #149 //cc @reynir @TheLortex

@hannesm
Copy link
Collaborator Author

hannesm commented Mar 1, 2023

I'm ready to get some review/comments about this. The commits should be understandable one-by-one. What it achieves is to check various things (policy is usable, unikernel matches policy, policy is smaller than the super-policy) locally; it also checks for client-bistro and client-remote-tls that the provided certificate chain validates (before initiating any network communication), and the albatross_provision_ca sign outputs a bundle (thus, the cat hello.pem user.pem > hello.bundle is no longer needed).

//cc @reynir @TheLortex

Copy link
Contributor

@reynir reynir 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 to me. There are some calls to Lwt.fail_with, and I'm curious if the exceptions are caught somewhere. If we catch them and print an error that's fine. By using Lwt.fail_with over failwith we lose the stack trace - and for these exceptions we likely don't want the stack traces.

src/vmm_core.ml Outdated Show resolved Hide resolved
src/vmm_core.ml Outdated Show resolved Hide resolved
Co-authored-by: Reynir Björnsson <reynir@reynir.dk>
@hannesm
Copy link
Collaborator Author

hannesm commented Mar 2, 2023

Looks good to me. There are some calls to Lwt.fail_with, and I'm curious if the exceptions are caught somewhere. If we catch them and print an error that's fine. By using Lwt.fail_with over failwith we lose the stack trace - and for these exceptions we likely don't want the stack traces.

I agree. The two modules where Lwt.fail_with is used are albatross_client_bistro and albatross_tls_common. I agree this should be revised. I as well think this can be part of this PR (but doesn't need to). To merge this PR, we should maybe test a sample deployment (to ensure there hasn't been any issue (e.g. using < instead of >)).

@reynir
Copy link
Contributor

reynir commented Mar 2, 2023

That sounds like a good way forward. I agree that revising exceptions doesn't have to be a part of this PR.

What we are not checking that we could check are the block commands - is the block size larger than allowed by the policy?

@hannesm hannesm merged commit 141463a into main Apr 9, 2023
@hannesm hannesm deleted the improve-ca branch April 9, 2023 12:21
hannesm added a commit to hannesm/opam-repository that referenced this pull request May 14, 2023
CHANGES:

This is a breaking release since the binaries to be installed have been revised
and merged. The `albatross-client-local` is now `albatross-client [--socket]`,
`albatross-provision-ca` is now `albatross-client sign` or `albatross-client
generate`. The `albatross-client-remote` is now `albatross-client certificate`.
The `albatross-client-bistro` is now `albatross-client <command>
--destination <host> --ca ca.pem --ca-key ca.pem --server-ca cacert.pem`.

And finally, `albatross-tls-inetd` is now `albatross-tls-endpoint --inetd`.

- Document TLS usage (robur-coop/albatross#155, robur-coop/albatross#156 @TheLortex @hannesm)
- Improve TLS experience by providing more reasonable error messages and apply
  more checks before establishing a TLS session (robur-coop/albatross#157 @hannesm @reynir)
- Slim down binaries:
  - remove albatross-stat-client binary (robur-coop/albatross#161 @hannesm)
  - move X509 parts out of Albatross_cli (robur-coop/albatross#158 @reynir)
  - merge albatross-tls-inetd with albatross-tls-endpoint (robur-coop/albatross#160 @hannesm)
  - merge albatross-client-inspect-dump into albatross-client (robur-coop/albatross#161 @hannesm)
  - merge albatross-provision-ca and albatross-provision-request into
    albatross-client (robur-coop/albatross#159 @reynir @hannesm)
  - merge albatross-client-remote and albatross-client-bistro and
    albatross-client-local into albatross-client (robur-coop/albatross#162 @hannesm)
- Check solo5 ABI tender version, allow multiple solo5 tenders to exist (named
  as solo5-{s,h}vt.<ABI> (robur-coop/albatross#163 @hannesm)
- Improve documentation and manpages (robur-coop/albatross#164 @hannesm)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants