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

Albatross work #19

Merged
merged 15 commits into from
Aug 15, 2024
Merged

Albatross work #19

merged 15 commits into from
Aug 15, 2024

Conversation

hannesm
Copy link
Contributor

@hannesm hannesm commented Aug 13, 2024

This takes the domain into consideration, and enables the /unikernel/info/ endpoint with the new interface.

Also removed some configuration items (multiple certificates in cert.pem, server.pem), since in the end we don't actually want them anymore.

This has been tested with our albatross server successfully :D

…t for the intermediate certificate)

also fix the /unikernel/info/<unikernel-name>
albatross.ml Outdated
TLS.close flow >|= fun () ->
Logs.err (fun m -> m "received error while reading: %a" TLS.pp_error e)

let query t ?domain ?(name = ".") cmd =
Copy link
Collaborator

@PizieDust PizieDust Aug 13, 2024

Choose a reason for hiding this comment

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

Some thoughts, @hannesm
let's say we have this endpoint /unikernel/info/domain:name
and somehow a user figures out they can edit the link to /unikernel/info/, could this function "potentially" return the result which would be the list of all unikernels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We never should have the domain exposed in an endpoint. The idea is that the domain will always be added by mollymawk depending on the user account. And the client can only choose the unikernel name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment, this is all in flux, since the old code and interface didn't deal with the policies properly. But I'm sure we will adapt the code to always use authentication and then to pass the domain properly to albatross :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I intended to say: good thinking, this is a very valid question. And indeed we need to be careful to not expose that.

As aside, what we developed in Marrakesh was cutting of the ":" from the unikernel names -- there we didn't bother about domains and the policies. Now, a /unikernel/info/:robur:roburtls won't work -- since albatross rejects the : in the name (only allows letters, digits, hyphen). In order to access the robur unikernels we need, as done in this PR, the intermediate certificate.

And you are very right, mollymawk has the permissions with the certificate and key it holds to create/destroy all the unikernels, so we really need to be careful to not expose functionality meant for other users within the HTTP interface. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay great. I'm happy to know that the domain is added later on at some point during processing and the user can never know what their domain is. I think information like this shouldn't be added to any user endpoints, maybe system admins (Robur)

Copy link
Contributor Author

@hannesm hannesm Aug 13, 2024

Choose a reason for hiding this comment

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

Now that we're talking about it, maybe the path to go is to make the domain argument mandatory :) I went ahead and pushed 7f7128b -- now everything where we have ~domain:"robur" is a TODO item.

@hannesm
Copy link
Contributor Author

hannesm commented Aug 15, 2024

let's merge this. CI is failing due to requiring a newer albatross, but a release is on the way, and this will solve itself soon enough.

@hannesm hannesm merged commit c005f22 into main Aug 15, 2024
1 of 2 checks passed
@hannesm hannesm deleted the albatross-work branch August 15, 2024 20:39
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.

3 participants