Add teos cli auth #262
Add teos cli auth #262
Conversation
|
Nice! Will give it a first pass this weekend. |
There was a problem hiding this comment.
First pass. Haven't checked tests yet.
Tests are failing on circle-ci. Looks like a missing dependency(grpcio-testing)
test/teos/unit/cli/test_teos_cli.py:5: in <module>
from grpc_testing import channel, strict_fake_time
E ModuleNotFoundError: No module named 'grpc_testing'
Docs will need to be updated to cover the following:
- How to add your own CERT in case you have it
- Where to place the tower CERT in a remote CLI setup
- How to re-generate the CERT for renewal (once added)
I also think the defaults for rpc user and password should be blank (if possible) and the tower should raise a warning if they are not set. Moreover, the tower should refuse to run if the user/pass are blank and the RPC host is not localhost.
| @@ -192,44 +199,44 @@ def generate_key(): | |||
| return PrivateKey() | |||
|
|
|||
| @staticmethod | |||
| def save_key_file(key, name, data_dir): | |||
| def save_crypto_file(crypto_data, name, data_dir): | |||
There was a problem hiding this comment.
I think we could call this save_bytes_file or save_file_bytes, given it has no actually cryptographic restriction.
| with open(os.path.join(data_dir, "{}.der".format(name)), "wb") as der_out: | ||
| der_out.write(key) | ||
| with open(os.path.join(data_dir, "{}".format(name)), "wb") as crypto_out: | ||
| crypto_out.write(crypto_data) | ||
|
|
||
| @staticmethod | ||
| def load_key_file(file_path): |
There was a problem hiding this comment.
The same applies to load_key_file, since currently the return messages make no sense for a certificate.
I think generalizing the messages and updating the name would do.
e.g.
"Key file path was expected, {} received" -> File path was expected, {} received"
|
|
||
|
|
||
| @staticmethod | ||
| def generate_cert_key(): |
There was a problem hiding this comment.
I'm really debating between using openssl to generate the cert or do it with code. Given the code addition is rather small, I don't think it a big deal though.
However, I'd greatly encourage to use ECDSA instead of RSA. Furthermore, we are currently using DER for the rest of the keys, so I think we should also use DER for consistency here.
There was a problem hiding this comment.
Got it, sounds good, I'll change to ECDSA. The reason I used PEM is because that's the format GRPC accepts. Sound okay for me to save the key/certificate as DER, and then just convert to PEM when we need to use them in GRPC?
There was a problem hiding this comment.
Yeah, I guess you can actually create Key and Cert objects with cryptography and the just call the pem / serialize_pem / whatever method gets the pem (I cannot recall atm) when passing the data to grpc.ssl_server_credentials.
If it ends up being too complicated it won't be worth it, so feel free to weight it and decide whatever you see fits best.
| pk = sk.public_key() | ||
|
|
||
| subject = issuer = x509.Name([ | ||
| x509.NameAttribute(NameOID.ORGANIZATION_NAME, u"Teos watchtower"), |
There was a problem hiding this comment.
The Eye of Satoshi maybe? I've never liked teos that much tbh, but that the acronym we've got.
|
|
||
| self.rpc_client = RPCClient(teos_rpc_host, teos_rpc_port) | ||
| rpc_cert = config.get("RPC_CERT") | ||
| rpc_user = config.get("RPC_USER") |
There was a problem hiding this comment.
rpc_user and rpc_password should be rewritable via command line params.
| @@ -142,6 +142,17 @@ def __init__(self, config, sk, logger, logging_port): | |||
| bitcoind_feed_params, | |||
| ) | |||
|
|
|||
| if not os.path.exists(self.config.get("RPC_CERT_KEY")): | |||
There was a problem hiding this comment.
I'm guessing this is here for the first bootstrap, but I think there's an uncovered edge case.
If the sk does not exists and the cert does exists, you will generate a new sk and load the old cert. That will not work.
I think you need to check that they both exist at the same time. There are basically 4 cases here:
- Cert + SK -> Check that the SK and the CERT match. If not, you need to generate a new pair**
- Cert + No SK -> The CERT is useless, you need to generate a new pair**
- No cert + SK -> You need to generate a CERT, but I'm pretty sure the CERT that the admin has won't be valid anymore, since the dates won't match (care to double check this?), so probably you need to generate a new pair**
- No CERT nor SK -> You need to generate a new pair**
** For the new pair generation, I think the best approach is not doing it straight-away. This is the same case as for when the CERT expires. A new RPC command (only available from localhost) needs to be added to re-create the CERT.
There was a problem hiding this comment.
So basically here we should return an error regarding the data being invalid / corrupted / not found (whatever sounds good) and ask the user to regenerate the CERT.
Tackles #230. Sets up a secure channel with TLS. (Generates a certificate for doing so.) And sends a user's provided username and password over the connection for verification.