server: add support for message authentication/authorization #46
Conversation
|
I could easily imagine the current approach might not be acceptable, but I'd be happy to make adjustments. Please see the commit message for additional explanation and rationale. I haven't added any tests yet, but "lein test" should still work, if a bit noisily, and the noise can be suppressed: In any case, assuming we can settle on an initial approach, I'll be happy to add tests and other desired enhancements. |
|
Oh, and I'd imagine before we're finished, this should include some updates to the manual. |
|
Oh, and in case it's not completely clear, the intention is that tools like lein might start specifying an auth-wrapper, i.e. |
|
I'll add a bit of feedback inline and I'll invite @nrepl/nrepl members to share their opinions as well. |
src/clojure/nrepl/server.clj
Outdated
| @@ -64,6 +64,31 @@ | |||
| s)) | |||
| % %)))) | |||
|
|
|||
| (defn no-auth-wrapper | |||
There was a problem hiding this comment.
I'm not sure I understand the purpose of this wrapper. I guess that if we don't want to use authorization we can simply pass nil or something along those lines.
There was a problem hiding this comment.
Assuming I understand, that'd be fine too. Were you saying that you just wanted to special case the "no auth" case at the point of use, i.e. put this code there guarded by a check whether the default-auth-wrapper is set, or are you saying that you don't want to warn at all when there's no protection?
There was a problem hiding this comment.
I'd rather not warn at all by default given the fact that now there are no clients supporting authentication and this warning is just going to be something annoying for the end users. But obviously selecting explicitly a no-auth option works in solving this as well, because if you chose it, then you're fine with it being absent. Generally I don't like no-something functionality - I think just need an auth-wrapper or middleware that can be configured additionally would be the best approach.
src/clojure/nrepl/server.clj
Outdated
| (println "Suppress warning with NREPL_SUPPRESS_DEFAULT_AUTH_WARNING=true"))) | ||
| handle-message) | ||
|
|
||
| (defn file-auth-wrapper |
There was a problem hiding this comment.
That's named file-auth-wrapper, but I don't see it doing anything with files.
There was a problem hiding this comment.
Oh, right, that's vestigial -- I'll fix it in the next revision.
|
On a general note - won't it be better to handle authorization with some built-in middleware? Seems to me that'd be more in line with the spirit of how we've been doing things so far. Overall the suggested approach seems to be simple, which is good. I definitely don't want to over-engineer things. I feel that we can't be emitting warnings in the absence of an auth token, as most clients won't support this for a while and that's going to be just annoying, even if it can be turned off. Generally in the beginning whatever form of authorization gets added should be opt-in and only way down the road we can think about turning this on by default (maybe). I hope this makes sense. |
src/clojure/nrepl/server.clj
Outdated
| @@ -107,8 +132,9 @@ | |||
| * :port — defaults to 0, which autoselects an open port | |||
| * :bind — bind address, by default \"::\" (falling back to \"localhost\" | |||
| if \"::\" isn't resolved by the underlying network stack) | |||
| * :auth-wrapper - defaults to the result of `(default-auth-wrapper)` | |||
There was a problem hiding this comment.
Potentially something like this should also be configurable via the command-line interface. Same goes for auth-token.
There was a problem hiding this comment.
Sounds reasonable -- I can investigate. If it's not straightforward, is that something you'd want in the first iteration, or is it possible that could be added later? (I agree that if it's feasible, it'd be better to have it now.)
There was a problem hiding this comment.
Take a look at the nrepl.cmdline ns - I think you'll find that adding this is going to be pretty simple. (but it's not something that necessarily has to be part of the first iteration)
There was a problem hiding this comment.
If i pursue it, what were you contemplating here? I could see doing something like what's done for --handler/--middleware (depending on how we resolve the middleware-related questions), but that still leaves the token. I assume we'll need to read it from an environment variable, file, stdin, or similar so that it's not visible to others on the system (via ps -au, etc.), which might suggest adding a custom auth handler to cmdline.
I suppose we could also consider defining a "standard", e.g. to look for ~/.nrepl-auth-token (or whatever) for a candidate token, as a default.
There was a problem hiding this comment.
For this type of authentication yes. On the server side everything's easy with respect to auth token and auth schemes. The real question is how to teach clients about different schemes. Guess the idea is to just hardcode the support for whatever auth schemes are known in the various clients.
src/clojure/nrepl/server.clj
Outdated
| (let [port (or port 0) | ||
| auth-wrapper (or auth-wrapper default-auth-wrapper) |
There was a problem hiding this comment.
The default should certainly be some no-op function.
There was a problem hiding this comment.
OK, perhaps that answers my other question above. And while it may not have to be addressed in tools.nrepl. I definitely think that the ecosystem should be secure by default, and so it should eventually require some effort to end up with a backdoor that bypasses all local authentication. At a minimum that might suggest ensuring that eventually all "front ends", i.e. leiningen, cider, the nrepl command line, etc. default to some kind of authentication.
There was a problem hiding this comment.
I wonder if it might be plausible to start with no changes to the defaults and/or external behavior (as you suggest), but add something to the README recommending authentication, and mention the desire to eventually change the default. Then at some point later, we could start warning to err about the impending change of the default (under the assumption that most consumers won't be parsing err, and won't be disrupted, but people may see it), and then finally, after that's been the norm for a while, change the default, along with an appropriate version change -- i.e. perhaps as a "breaking" change.
There was a problem hiding this comment.
Yep, that's exactly the approach I'd take. Big changes should happen gradually and need to be tested extensively first. We also need to give time to clients to catch up with this.
There was a problem hiding this comment.
With respect to middleware -- assuming I understand your comments correctly, I treated the authentication independently under the supposition that authentication is somewhat different. i.e. it's something that should "go first", so that nothing else can be added before it that accidentally or intentionally acts on incoming, unauthenticated data. Treating it independently was also a simplification, trying to make it easier for someone to convince themselves that they (or someone else) hadn't accidentally or intentionally, broken or bypassed intended restrictions.
Along those same lines, I was (rightly or wrongly) thinking about the decision to accept a message as potentially independent of the decision about what to do with it once it's been accepted (a la ssh/pam, etc.).
I suppose the alternative might be to just make authentication a normal wrapper, and add loud documentation about always "tacking it back on to the front" if you use your own middleware, or if you add your own wrappers in front of the default wrapper. (Of course in the latter case, you'd end up authenticating the same message multiple times.)
And one reason I'd provided the default-auth-wrapper (even if the default were identity for now) is so that people could easily wrap the default handler with their own additional policies.
Yeah, I see this point. Generally I think that in practice that's unlikely to be a problem as you can obviously intercept any message in an authentication middleware, so unless someone explicitly disables it we're in business. But I'm not insisting on a middleware approach. I just seems to me in line with everything we've done some far (e.g. even sessions are middleware and they could have easily been something special).
You'll have to elaborate on this a bit. We still have to authenticate every message regardless of the approach taken, so I'm a bit puzzled by this comment.
For the middleware approach this seems perfectly fine to me. If we go with the wrapper approach probably we can just make this some var you can pass on server-init. I'm thinking we can also have a configurable middleware that could support different auth schemes, but that would probably be an overkill initially. Long story short - I try to be really careful with the approach we take, so that we're certain we select something that's going to be flexible, get the job done and relatively consistent with how we've operated so far. |
I just meant that I did this: so that it was really clear that authentication happens before anything else, and you can't side-step it. Whether or not that's what we want, is of course, another question, but that's what I was thinking at the time. (The vague (possibly inappropriate) ssh analog being that you can't run any commands until you've been allowed access, perhaps passed the whole pam stack, etc.). And with respect to your other suggestions, I have a suspicion I don't completely understand the arrangement you're contemplating. I was wondering if you might be able to sketch it out a bit more. i.e. What would the approach(es) you've been thinking of look like? Thanks |
I see.
What I was thinking about was:
I think that's pretty similar to what you're proposing with the slight difference it's a middleware instead of something hardcoded in the server. Once again - I'm not 100% sure which approach would be better. At least superficially your idea seems to be a bit simpler, but on the other hand - not very consistent with with how most functionality has been implemented so far. |
|
Bozhidar Batsov <notifications@github.com> writes:
For this type of authentication yes. On the server side everything's
easy with respect to auth token and auth schemes. The real question is
how to teach clients about different schemes. Guess the idea is to
just hardcode the support for whatever auth schemes are known in the
various clients.
Hmm, I'd vaguely imagined that past a certain point, tools.nrepl might
not necessarily be involved. i.e. the critical thing is that
tools.nrepl provide a hook of some kind, and once that's available (in
whatever form), then leiningen, cider, et. al. might decide on a
specification like:
- messages must have an :auth token
- check NREPL_AUTH_TOKEN, in the environment; if set, use it
- check NREPL_AUTH_TOKEN_FILE in the environment; if set, use it
- otherwise look for PROJECT_ROOT/.nrepl-auth-token
- otherwise look for ~/.nrepl-auth-token
- give up.
And so while tools.nrepl *could* be involved and might provide a
built-in function implementing the "lein/cider/whoever strategy", it
needn't be, and/or can wait until they've stabilized their
specification before adopting anything officially.
So I'd planned to badger technomancy (and the cider devs) next, once we
had the hook, but I'm happy to proceed in whatever way seems best, and
it'd also be perfectly fine if tools.nrepl does want to provide
something like the above "out of the box".
If the latter, then I'd be happy to double-check with leiningen and
cider about any proposed semantics.
Oh, and I could also imagine a path where tools.nrepl eventually *only*
provides the hook, and makes the :auth-wrapper/whatever argument
required, so that you have to specify a strategy, even if you just say
"identity". That way the decision is explicit, and obvious.
Thanks again
--
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4
|
Btw, it's just nREPL (
I guess you don't know that I'm the author of CIDER. 😉 That's why from the very beginning I've been thinking about this end-to-end. And I happen to be quite familiar with most of the other nREPL clients out there. :-) |
Hah, no I didn't. And well, that's certainly convenient. So backing up a bit. I'll be ecstatic if we just make it possible for people to enable some form of solid authentication for lein/cider/etc. Anything more I'd just consider a bonus -- and with respect to a lot of the details, "if you're happy, I'm happy". Given that, what would you like this first version to look like? Would it resolve most of the questions if I were to change the auth implementation to be a middleware, assuming I understand what we mean by that, and then provide a default auth-middleware stack that's empty, along with a Then, once we're happy with that, we can perhaps ping technomancy, if he's not already paying attention... |
Yep, that'd be a good start. I'm guessing that with the middleware approach we can have some param as part of the server init that controls wether the middleware does something or is an no-op.
Sounds like a plan to me. There's also my small https://github.com/nrepl/lein-nrepl where we can add support for this before it makes it to upstream leiningen. Changes there certainly take a while to happen, so this might be a good stop-gap measure. |
|
Regarding the middleware approach, were you just thinking that we might start with then maybe later change the default to something like: or were you imagining the more complicated arrangement we have on the default-middlewares side, i.e. linearization, etc.? (If the latter, I'm a bit wary of any more complexity than we really have to have when auth is involved.) |
|
Yeah, your suggestion makes sense to me. I'm assuming you plan to make the auth-middlewares run before every other middleware, right? Btw, it might be good to use |
|
Bozhidar Batsov <notifications@github.com> writes:
Yeah, your suggestion makes sense to me. I'm assuming you plan to make
the auth-middlewares run before every other middleware, right?
Yep, they'd always go first.
Btw, it might be good to use `authentication` instead of `auth` in the
code, as some people also brought up the issue of authorization and
`auth` can be interpreted as referring to both.
Hah, I'd also been pondering that -- I'll change it. Would you also be
inclined toward `NREPL_AUTHORIZATION_TOKEN` rather than
`NREPL_AUTH_TOKEN`, etc., and/or an :authorization key in the message
rather than :auth?
|
|
Well, seems to me that'd be better - especially given the fact that i called it |
|
Bozhidar Batsov <notifications@github.com> writes:
Well, seems to me that'd be better - especially given the fact that i
called it `authentication` and you called it `authorization`. :D I
just assumed that we'd consider the token just an authentication
mechanism and if you are authenticated (have the right token) you're
authorized to do anything, but I guess your perspective on this is
different - the presence of a token authorizes you to do anything.
Acually I've just been mostly careless about the terminology so far,
though I *did* originally choose :auth to side-step the question.
In any case, for this first pass, yes, I was assuming that the presence
of a valid token provides carte blanche, authorizing any action.
I suppose it could be used for authentication if there were some way to
map specific tokens to permissions on the server side, but if I'd been
thinking about anything other than blanket authorization, I might well
have approached the solution differently.
|
|
I think blanket authorization will be fine, so let's not overthink this. After all any form of authorization is going to be infinitely better than none. :-) In an ideal world it would have been nice to be able to optionally tie auth tokens to the ability to execute only certain middleware ops, but that might be a bit excessive, so let's go with something simple. |
|
Bozhidar Batsov <notifications@github.com> writes:
I think blanket authorization will be fine, so let's not overthink
this. After all any form of authorization is going to be infinitely
better than none. :-) In an ideal world it would have been nice to be
able to optionally tie auth tokens to the ability to execute only
certain middleware ops, but that might be a bit excessive, so let's go
with something simple.
Sounds good. I wondered about keeping the shorter :auth for the key
since it'll be in every message, but probably not significant either
way. I'm fine with whatever you prefer.
Also, right now I have an additional :explanation key for errors. Is
that reasonable, or should it be something else?
(t/send transport
(response-for msg :status
#{:error :unauthorized :done}
:explanation "invalid :auth token")))))
(And with respect to the future -- wouldn't be hard to adapt later,
regardless -- even if that required the addition of some kind of
message versioning for some change in semantics we didn't want to work
around some other way, i.e. ":v 1" messages, etc.)
|
I'm fine with the shorter name as well, provided the middleware is well documented.
We always put errors in the status set, so I guess you can have there |
|
Bozhidar Batsov <notifications@github.com> writes:
> Sounds good. I wondered about keeping the shorter :auth for the key
since it'll be in every message, but probably not significant either
way. I'm fine with whatever you prefer.
I'm fine with the shorter name as well, provided the middleware is well documented.
OK, so sounds like you prefer :auth in the message, and authorization
everywhere else, i.e.:
(defn find-token
[]
(or (System/getenv "NREPL_AUTHORIZATION_TOKEN")
(some-> (System/getenv "NREPL_AUTHORIZATION_TOKEN_FILE") slurp)
(some-> (try
(slurp (File. (System/getProperty "user.home") ".nrepl-authorization-token"))
(catch FileNotFoundException ex nil)))))
We always put errors in the status set, so I guess you can have there
`:unauthorized :no-auth-token`, `:unauthorized :invalid-auth-token`.
Ahh, ok. I'll rearrange that.
|
|
Bozhidar Batsov <notifications@github.com> writes:
Yeah, your suggestion makes sense to me. I'm assuming you plan to make
the auth-middlewares run before every other middleware, right?
I just realized that maybe I didn't quite understand what you preferred
here, but I was somewhat thinking about ring's wrappers, and here's what
I currently have (which may or may not be quite what we want):
(defn default-authorization-wrapper
"Returns identity (i.e. no access restriction), matching previous
nREPL behavior, but is expected to eventually return
nrepl.middleware.auth/require-nrepl-authorization-token."
[]
identity)
...
(defn start-server
"Starts a socket-based nREPL server. Configuration options include:
...
* :authorization-wrapper - defaults to `(default-authorization-wrapper)`
which currently does nothing;
nrepl.middleware.auth/require-nrepl-auth-token is recommended
when the client supports it.
* :handler — the nREPL message handler to use for each incoming connection;
only receives messages permitted by the :authorization-wrapper;
defaults to `(default-handler)`.
...
authorization-wrapper (or authorization-wrapper (default-authorization-wrapper))
handler (or handler (default-handler))
...
server (Server. ss
(.getLocalPort ss)
(atom #{})
(or transport-fn t/bencode)
greeting-fn
(authorization-wrapper handler))]
So the caller can specify their own wrapper, use the default one, or
wrap the default one before passing it in, and the authorization-wrapper
always "goes first".
|
|
That seems reasonable to me. |
|
How is the existing |
|
Bozhidar Batsov <notifications@github.com> writes:
No need to support var references.
Got it.
They seem pretty similar now, so I guess you can just add `die` to the
existing function and that'd be enough (unless I'm missing something).
Indeed, now that I notice that resolve should do the same thing as
ns-resolve when given a fully qualified symbol, then they're very
similar, excepting the (possibly) friendlier error handling. I can
integrate them -- though we could also leave it alone if we don't want
the additional complexity in exchange for that.
|
|
Bozhidar Batsov <notifications@github.com> writes:
Yeah, that makes sense.
Btw, in the mean time I've added 9129bf6, so you should leverage `nrepl.config` to some extent in this patch.
Looking at this, I see the (handler) invocation was dropped, i.e. this:
handler (if handler (handler) (build-handler middleware))
changed to this:
handler (or handler (build-handler middleware))
Just wanted to double-check that it was intentional.
9129bf6#diff-157defd03be20ee96b1f635d628a73a3L171
Thanks
|
rlbdv
force-pushed
the
rlbdv/add-nrepl-auth
branch
2 times, most recently
from
2e5de10
to
67d7c06
Compare
|
OK, I've pushed an update, but I haven't changed the authorization code much yet (with respect to our current discussion). Instead, I've broken the unrelated changes out into separate commits, before the the auth changes, and I've rebased onto current master. Since the unrelated changes had accumulated, I thought it'd be better to consider them on their own merits. Please let me know if you'd like me to change or omit any of them, or cherry-pick them if you like, and I'll rebase again. |
rlbdv
changed the title
rlbdv
force-pushed
the
rlbdv/add-nrepl-auth
branch
from
67d7c06
to
6439e0d
Compare
rlbdv
changed the title
|
I've rebased onto current master and split the auth work into two commits. The first only adds the Oh, and I switched to |
src/clojure/nrepl/cmdline.clj
Outdated
| @@ -157,7 +157,7 @@ | |||
| "Require and resolve `thing` | |||
| `thing` can be a string or a symbol." | |||
| [thing] | |||
| (let [thing (symbol thing)] | |||
| (let [thing (edn/read-string thing)] | |||
There was a problem hiding this comment.
Won't this blow up if thing is a symbol?
There was a problem hiding this comment.
Yep, thanks, should be fixed.
rlbdv
force-pushed
the
rlbdv/add-nrepl-auth
branch
from
6439e0d
to
ded1604
Compare
My only real concern about this is the use of the env variable there. If you only move the configuration to rely only on
That's fine. I was thinking we might call this |
|
Bozhidar Batsov <notifications@github.com> writes:
That's fine. I was thinking we might call this `message-filter` or
something along those lines, as it's actually even more generic that
`auth-wrapper` gives it credit. :-) Ultimately we can advertise this
as a way to go to the very bottom of the middleware stack.
Agreed -- :message-filter sounds like it might be more appropriate...
In any case, I've been thinking about this a bit too -- about the fact
that support for :message-filter (or whatever), while possibly
convenient, and perhaps a touch potentially safer (i.e. harder to
avoid), may not really be the key thing.
That is we could (of course) do what's needed with just the existing
:handler or :middleware support, and some policy that all the relevant
parties can agree to (perhaps you were suggesting this earlier). Given
a policy, could also provide a middleware function that implements it,
so that clojure-based tools can just use it.
But at least right, now, I suspect that the policy's going to end up
including environment variables (since it's one of the safe ways to
provide a value directly -- as opposed to the command line), and for
clojure-based tools, I imagine it'd be a lot more convenient to just
rely on some function that we provide, say:
(or (some-> command-line-token-file maybe-slurp)
(System/getenv "FOO_AUTHORIZATION_TOKEN")
(some-> (System/getenv "FOO_AUTHORIZATION_TOKEN_FILE") slurp)
(maybe-slurp current-directory-token-file)
(maybe-slurp home-directory-token-file)
(maybe-slurp system-level-etc-ish-token-file)))
...or whatever policy at least nrepl, cider, and lein can agree on.
Then they don't have to write the code themselves, nor track any changes
to the policy.
That said, it occurs to me that, ignoring the "too many small artifacts"
concern, it would be better if this function were in its own library so
that a client that wants to use it doesn't have to haul in the whole
server.
Thanks again
|
|
But if the code to implement the policy were simple enough (I'd certainly hope so), we could just provide it for people to copy -- I'm not remotely excited by the prospect of a library containing a 10 line function. |
|
(...and honestly, a system-level /etc based token file seems unlikely to be all that useful in this case.) |
|
Add a server :auth-wrapper option that can specify a wrapper for the
existing :handler, supporting access control checks that cannot easily
be bypassed, intentionally or accidentally. Allow specification of
the wrapper from the command line via --auth-wrapper some/thing.
The intent is for all tools to eventually specify some suitable
wrapper, and perhaps even to change the server to default to a
specific :auth-wrapper that requires some form of authorization (in a
backward-incompatible release), since running an nrepl server
currently leaves your host open to local exploits, bypassing all
system authentication (password or otherwise) for your account[1].
[1] If you're lucky, they'll run xmeltdown; if you're not, rm -rf
"$HOME"
Provide a require-nrepl-authorization-token wrapper that requires all
incoming messages to have an :auth key whose value matches a token
retrieved as:
- the value of NREPL_AUTHORIZATION_TOKEN in the environment, or
- the content of the file indicated by of
NREPL_AUTHORIZATION_TOKEN_FILE in the environment, or
- the content of ~/.nrepl/authorization-token
See the docstring for additional information.
rlbdv
force-pushed
the
rlbdv/add-nrepl-auth
branch
from
ded1604
to
c2199e2
Compare
|
Bozhidar Batsov <notifications@github.com> writes:
Great points! The more I think about this, the more I realize how
careful we have to be with its design. I'd still prefer to use nREPL's
config files over env variables, though, but that's a relatively small
implementation detail.
I'm not absolutely certain either way myself, and of course it's always
something we can figure out (and add) later if we're not sure, but at a
minimum (and please consider this an incomplete idea), I've imagined
that I might want to provide some standard setting like
NREPL_DISALLOW_UNRESTRICTED_ACCESS that everything respects, and that
would cause nrepl to just refuse to operate when the auth system isn't
active (whatever that ends up meaning).
i.e. I know we don't want to make auth the default yet, but it'd be nice
if there were a way to opt-in to requiring it.
Though now that I think about it, I suppose the setting could just go in
~/.nrepl -- perhaps that'd be sufficient.
|
|
I hate to pollute the thread, but since it's related, can I ask where SSL/TLS support would be discussed? |
|
Jason Whitlark <notifications@github.com> writes:
I hate to pollute the thread, but since it's related, can I ask where
SSL/TLS support would be discussed?
Not sure -- I'm not planning to try to address that now. Perhaps a new
issue or something similar?
And apologies for the inactivity -- I've been tied up by various things
recently, particularly Emacs for the buster release, but I hope to get
back to this in the next few weeks.
|
|
@rlbdv Don't worry - in the mean time Lein 2.8.3 came out and it bundles nREPL 0.5.3, which is a great victory as now Lein natively supports the new nREPL configuration files. We've also had @cgrand join our team and he has been working on some really great improvements for the next release.
Yeah, this seems unrelated. I guess we can have some encrypted transports, but that's definitely not something high priority for me right now. In the mean time using the HTTPS transport (Drawbridge) seems like a decent workaround, or just tunnelling a normal unencrypted connection over SSH. |
|
autogenerated with https://github.com/MalloZup/doghub: pr inactive since 200 days. Please update the PR or close it |
|
Just as an update, I set this aside to pursue #204 first, and may (or may not) return to this once that one's resolved. |
|
@rlbdv Should we close this one? It has been dormant for quite a while. |
|
I think the filesystem socket support we added is sufficient for me for now, so perhaps -- I'm not likely to pursue it further right now, though I suppose it might still be interesting for situations where filesystem sockets aren't sufficient. |
No description provided.