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

Darix/client cert support #14183

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

darix
Copy link
Member

@darix darix commented Apr 18, 2023

This is not meant as a final implementation. all those TODOs need solving. This is just a rough untested sketch to see how one might be able to do this.

the basic idea is to talk to the back end via TLS and use client certificate for authentication.

@github-actions github-actions bot added the Frontend Things related to the OBS RoR app label Apr 18, 2023
@darix darix force-pushed the darix/client-cert-support branch from dd87a87 to 85b5881 Compare April 18, 2023 13:48
@hennevogel hennevogel marked this pull request as draft April 18, 2023 13:48
@darix darix requested a review from hennevogel April 18, 2023 13:54
@hennevogel
Copy link
Member

the basic idea is to talk to the back end via TLS and use client certificate for authentication.

JFYI: The reason to go the client certificate route and not SSL + Auth is because socket passing is not implemented for some parts of the backend.

@hennevogel
Copy link
Member

Just some general architecturla thoughts:

  • I do like the simplicity on the frontend and how low level this is ❤️
  • I don't like the reliance on just another tool (ha-proxy on front of the backend) to be able to do this. Especially for releases.
  • I don't like the reliance of specially crafted certificates which will mean SSL documentation (probably distro specific and stuff)

@@ -22,13 +24,52 @@ class Connection
end
end

def self.ssl_options()
unless @@ssl_options
Copy link
Member

Choose a reason for hiding this comment

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

It'll probably be better to do this in an initializer once and store this in a class accessor. So this is per rails process (which we have many of) instead of per instantiated class (we do this often just for one call).

Copy link
Member Author

Choose a reason for hiding this comment

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

as i said ... I am not sure what the best place is :)

but we also need to implement a clean way to reload the cert without restarting the app (ideally)

Copy link
Member

Choose a reason for hiding this comment

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

but we also need to implement a clean way to reload the cert without restarting the app (ideally)

What for?

ssl_options[:cert] = OpenSSL::X509::Certificate.new(cert_data)

# if you concatenate key and cert into one file you do not need to pass in source_client_key
# and we can reuse the cert_data here.
Copy link
Member

Choose a reason for hiding this comment

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

Or we can just settle and document for one of those options...

@darix
Copy link
Member Author

darix commented Apr 18, 2023

  • I don't like the reliance on just another tool (ha-proxy on front of the backend) to be able to do this. Especially for releases.

the haproxy is only a workaround until we find a solution for the file descriptor passing problem in the backend.

  • I don't like the reliance of specially crafted certificates which will mean SSL documentation (probably distro specific and stuff)

the certs arent distro specific. a config option would be a good idea for those paths of course. and you can roll out certs with many different tools.

@hennevogel
Copy link
Member

hennevogel commented Apr 18, 2023

the haproxy is only a workaround until we find a solution for the file descriptor passing problem in the backend.

I understood "the problem" is that this is just not implemented? Why do we workaround this with code and ha-proxy setup etc. instead of doing this then? :)

I mean if we are going to throw this away and go with SSL + auth to talk to the backend in the future. We should make up our mind...

@darix
Copy link
Member Author

darix commented Apr 18, 2023

  1. we want everything to use SSL for talking to other parts.
  2. some parts of the backend can not easily implement this natively due to the use of socket passing. for those parts we will move the TLS part for now to haproxy. all other parts implement it natively.
  3. this does not change the fact that the rails part needs to talk with TLS and client cert to the other bits and pieces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend Things related to the OBS RoR app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants