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

Support TLS server sockets #266

Closed
ivarref opened this issue Feb 17, 2022 · 8 comments
Closed

Support TLS server sockets #266

ivarref opened this issue Feb 17, 2022 · 8 comments

Comments

@ivarref
Copy link
Contributor

ivarref commented Feb 17, 2022

Is your feature request related to a problem? Please describe.

Currently our containers are running inside private virtual networks in Azure. There is no SSH available on the containers' hosts. Thus we have to bind to 0.0.0.0 when starting nREPL servers. If someone gets access to the virtual network, they will also have access to all backends that exposes nREPL servers.

Describe the solution you'd like

I'd like to be able to run nrepl.server/start-server with either :tls-keys-str or :tls-keys-file to further strengthen security.

Describe alternatives you've considered

I've considered running apache-mina on the containers. This adds additional dependencies. The JVM already supports TLS (no extra dependencies needed).

Additional context

I have a working proof of concept at ivarref/nrepl and ivarref/cert-friend (the latter only for writing keys). It's based on aphyr/less-awful-ssl, but updated to use TLS v1.3, as well as adding some string utils.

  1. Install ivarref/nrepl version 0.9.0-SNAPSHOT into the local repository:
$ git clone git@github.com:ivarref/nrepl.git && cd nrepl && lein install
  1. Generate server and client keys:
$ clojure -Sdeps '{:deps {com.github.ivarref/cert-friend {:sha "824a799d0bdf1ec6fa74f0905d7e22dc2d63bb38" :git/url "https://github.com/ivarref/cert-friend.git"}}}' \
-X com.github.ivarref.cert-friend/write-certs
# Wrote server.keys
# Wrote client.keys
  1. Start a nREPL server with TLS:
$ clj -Sdeps '{:deps {nrepl/nrepl {:mvn/version "0.9.0-SNAPSHOT"}}}' \
-M -m nrepl.cmdline --port 5633 --tls-keys-file server.keys --headless
# nREPL server started on port 5633 on host localhost - nrepls://localhost:5633

Notice the s in nrepls.

  1. Start a TLS client proxy:
$ clj -Sdeps '{:deps {nrepl/nrepl {:mvn/version "0.9.0-SNAPSHOT"}}}' \
-X nrepl.tls-client-proxy/start-tls-proxy \
:port 5600 \
:remote-port 5633 \
:remote-host '"127.0.0.1"' \
:tls-keys-file '"client.keys"'
# Started TLS proxy at 127.0.0.1:5600. Proxying to 127.0.0.1:5633.
  1. Connect to the TLS client proxy (127.0.0.1:5600) using your favorite editor/IDE.

Thanks and kind regards.

@bbatsov
Copy link
Contributor

bbatsov commented Feb 21, 2022

I think it's a good idea in general and I see your prototype doesn't introduce any deps, so I'd consider something like this for upstream inclusion. My main concern is the long-term maintenance of this functionality, though, as TLS is not exactly my specialty. :-)

We'll also need to document this extensively (both the rationale and the usage) for upstream inclusion.

@ivarref
Copy link
Contributor Author

ivarref commented Apr 1, 2022

Thanks @bbatsov

I've cleaned up the code a bit. My speciality is also not TLS unfortunately.

As long as the Java API does not change, I think that "upgrading" the TLS protocol used is as easy as updating TLSv1.3 three places in tls.clj.
Aphyr's less-awful-ssl library, which this pull request's code is based on, is still on TLSv1.2 as the highest supported protocol.

I'm no expert, but from what I understand elliptic curve keys have become more mainstream since Aphyr's original version. They are shorter and faster. Adding support for them (or another keytype that Java supports), was as easy as adding a string to a vector here.

With regard to this I'm not all that worried that maintaining this will be too hard. At least insofar as maintaining only means keeping used protocol versions up to date.

I will try to document both rationale and usage.

@bbatsov
Copy link
Contributor

bbatsov commented Apr 2, 2022

Sounds like a plan to me. Thanks for the update and for working on this!

@bbatsov
Copy link
Contributor

bbatsov commented May 15, 2022

@ivarref Any progress here?

@ivarref
Copy link
Contributor Author

ivarref commented Jun 3, 2022

Hi @bbatsov

Sorry for the long delay again. I have been sick lately. I haven't forgotten about this though.
In the mean time CVE-2022-21449: Psychic Signatures in Java happened. I hope it's safe to assume that most people have upgraded their JVMs by now.

I want to make the following changes/improvements:

  • Use a more standard format of the certs/keys, to make the combined file compatible with a cat of openssl output.
  • Set the key file to mode 0400 if possible.
  • Document.
  • Add tests.

I hope to get time for this soon.
Thanks for your patience.

@bbatsov
Copy link
Contributor

bbatsov commented Jun 3, 2022

No rush!

@ivarref ivarref mentioned this issue Sep 1, 2022
5 tasks
@ivarref
Copy link
Contributor Author

ivarref commented Sep 1, 2022

Hi again @bbatsov

I've created a pull request:

#283

What do you think?

Thanks again!

Edit:
It has a few tests verifying the TLS behaviour.

And documents how it is to be used.

make test eastwood kondo cljfmt works, but cloverage does not.

Edit 2:
Version numbers for nREPL needs to updated in tls.adoc and CHANGELOG.md, otherwise I don't know about other things that needs to be changed/updated.

ivarref added a commit to ivarref/nrepl that referenced this issue Sep 6, 2022
ivarref added a commit to ivarref/nrepl that referenced this issue Sep 7, 2022
ivarref added a commit to ivarref/nrepl that referenced this issue Sep 12, 2022
ivarref added a commit to ivarref/nrepl that referenced this issue Sep 14, 2022
ivarref added a commit to ivarref/nrepl that referenced this issue Sep 15, 2022
ivarref added a commit to ivarref/nrepl that referenced this issue Sep 15, 2022
ivarref added a commit to ivarref/nrepl that referenced this issue Sep 15, 2022
bbatsov pushed a commit that referenced this issue Sep 16, 2022
@bbatsov
Copy link
Contributor

bbatsov commented Oct 19, 2022

Implemented in #283.

@bbatsov bbatsov closed this as completed Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants