Improve security around access keys

[Legogris]

To summarize my understanding of sparkos auth:

• user/password for http basic auth: Configured via sparko-login=user:pass
• master access key via X-Access header, URL query param: deterministic hmac from sparko-login
• Cookie auth: contains sparko=login, ttl 30d
• additional keys, set in cleartext via sparko-keys=[secret: [permission, ];]

The problems with this approach:

• Secrets need to be set in cleartext in configuration, and kept in memory during the lifetime of the process
• The access key needs to either be fetched from the running instance (via logs) or derived separately by the user form credentials

Since keys can give full control to a users lightning wallet and instance, I think it's important to minimize the exposure of these.

One can of course come up with more elaborate schemes, but here's one less-effort solution that could mitigate this:

• Reuse bitcoind's approach with auth cookies: https://github.com/bitcoin/bitcoin/blob/master/share/rpcauth/rpcauth.py for both user/password and additional keys
  • This is similar to the current implementation for the access key in sparko, but instead of setting the secret in config and generate the hmac at runtime, sparko would be configured with the hmac (generated with e.g. the above tool by the operator), and then validate that towards user input at validation time

IMO this is an easy mean to significantly reduce the exposure of credentials in memory and on disk.

If you agree I could take a stab at an implementation @fiatjaf

[fiatjaf]

I'm not sure I like the idea of generating keys on the fly. So every time you restart Sparko your keys will change?

[Legogris]

I'm not sure I like the idea of generating keys on the fly. So every time you restart Sparko your keys will change?

I think I wasn't clear or you misunderstood me - it sounds like we agree :) following up from https://github.com/fiatjaf/sparko/pull/10#issuecomment-764096518:

But following up on #11 I think this works if we add a new optional option, sparko-keyhashes=, that will work just like sparko-keys=, but for people who don't want to type the key on their config file, they can just type a hash.

And on memory we will just store the hashes. For sparko-keys we hash them all. For sparko-keyhashes we just use the hash the user has provided.

Then when someone does a call attempt we hash the key they sent and check against the key hashes we have in memory. What do you think?

That's pretty much excactly what I had in mind above - not generating anything on startup, just hashing the password as it's received from the user and comparing it to the configured hash. The hash is computed outside of sparko itself (this is where i was proposing to reuse bitcoind's rpc auth, as sparko users are likely to already use and be familiar with that one)

[Legogris]

With that in mind, do you agree that the current access-key derived from a username/password should be removed and instead users will configure sparko-keyhashes?

Should even the username/cleartext password be deprecated and replaced by that..? What do you think?

[fiatjaf]

I think we should still support the old configurations, but add new ones that accept hashes instead of plaintext stuff.

[Legogris]

OK - how do you feel about the need for the user to extract the generated password from application logs that print it on every startup? I get that it's not desirable to break existing API and that it's ultimately the users choice... It's just that this (BTC lightning wallet) is precisely the kind of software where I think it most likely will happen that some users lose funds due to misconfiguration or getting hacked, and especially users who aren't well-versed in hosting or secrets management will not understand why certain things would put them at risk and why..

So I argue for making it easier/more accessible using things the secure way. Keeping exactly the current state of things makes the very insecure thing a lot easier for someone who's a bit green I think.

But OTOH, maybe the logging part can be completely removed and instead a cli is made availalke to generate the access key from credentials offline and that would remove the biggest concern here?

[fiatjaf]

I think that breaks everything. My goal with Sparko is to use it as an API server for your node, so external apps can use it. Regenerating an API Key on every restart breaks that completely. I think we can provide that option, but we shouldn't prevent the users from using in ways that are less secure.

Ultimately I think these security concerns aren't very valid in the specific case of c-lightning, because if someone has access to your config file they also have access to your RPC socket, so they can already control all your funds, right? The logs are a bit worse.

[Legogris]

Let me see if I can make a PR to make it more clear, I'm proposing specifically that nothing should be generated or needed to be extracted on startup. The offline generation of hashes using a tool would only be done once when setting up a new key, and every time a key is rotated (not every startup, not every time using).

Same principle as Linux passwords.

There are some scenarios this would protect against that this protects again that simply removing secrets from logs (which I agree is a bigger issue and less disruptive fix)

• User backs up configuration file including secrets, backup gets compromised
• User saves config to their workstation, which gets compromised by malware
• Attacker compromises system, gets access to socket/port and configuration file

With secrets in cleartext, you're instantly KO.

With salted hashes, these are only useful for verification. the attacker would need to crack these to be able to impersonate a user - leaking the configuration file would no longer mean access to the RPC port means a compromise.

[fiatjaf]

Ok, I guess I'm misunderstanding half of what you say, but you apparently know what you're doing so it should be fine.