epee: add SSL support #4054
epee: add SSL support #4054
Conversation
|
NACK. Monero needs to avoid maintaining this feature for RPC communication, and leave it to outside projects. The SSL is not "transparent" because there is no way for the clients to automagically verify the certificates. Some user intervention is required, and its easier to leave the configuration hell to projects dedicated to this problem area. If Monero needs "opportunistic encryption" for P2P communication, it should follow the corresponding Bitcoin BIP, use CurveCP, or come up with something similar. SSL is a beast for this purpose, especially since the P2P links are never going to be authenticated (if they were, I would use SSH tunneling or something else not SSL). |
|
--{rpc,daemon,p2p}-allowed-certificates allows a client/server to authenticate the certs. I do not understand what you mean by: The SSL is not "transparent". If you leave it to using other tools, pretty much nooone will be using it and we'll just DoS ourselves (especially since both sides have to be using it, and I don't see how that'll happen for P2P). |
|
@moneromooo-monero If I'm not mistaken, by transparent I think he means something more along the lines of CurveZMQ (which is something I've wanted to do with kovri reseed for a while now). |
I have found it. Can you explain why you think it is better to use this than SSL ? |
|
About the other suggestions: CurveCP says it's not ready for use yet, and making up my own system... not really, it's a recipe for failure. |
The context was server SSL for RPC. The setup cannot be automatic because the server certificate cannot be verified by the client unless the server operator takes some outside action (signed by CA, distributing the certificate, etc). So encryption will always require some additional action and know-how (by at least the server operator). If Monero assumes that the SSL certificate is valid is on first use, its arguable worse than having no built-in SSL because the same target users may be unaware that normal privacy and authentication protections of SSL have been potentially removed via MitM. Combined with the many whizz-bang options of SSL, I still think its best to steer people towards software whose focus is solving this issue. Perhaps by default the client could refuse connections whose certificates could not be verified? I'm still not sure due to the additional maintenance burden this places on the project (Monero is now maintaining a subset of stunnel features, etc.). EDIT: Monero already warns users about binding to an external interface and steers them towards stunnel. So they are making a conscious decision to ignore that advice currently. And complaints about setup on these outside processes is entirely the point - to do it "correctly" is difficult.
This still has a similar issue as above (typical key distribution), but its certainly more simple than SSL and uses crypto that Monero relies on already. There is no key revocation, but that never worked for SSL (one of the many complicating anti-features). Also, this reminds me, isn't the HTTP RPC deprecated? I suppose that isn't practical?
DoS ourselves? I do not understand what that means in this context. For P2P I wasn't against some opportunistic encryption scheme, just against using SSL for this purpose. I don't think its the correct protocol - we'd want something as simple as possible for this purpose IMO. Or perhaps we should reach out to the Bitcoin Core developers to see why they developed their own protocol for this purpose. |
That is preposterous. You are claiming the user using --daemon-allowed-certificates is taking an outside action, wherewas getting a CA to sign a certificate is not. Of course a server/client can't verify a certificate without any action. --daemon-allowed-certificates just happens to be a very simple way to do this.
Only the MITM node(s) can read/modify the traffic, as opposed to anything else on the path. And that's only if you decide to accept any cert.
That seems maybe like a good idea.
No, it was supposed to become deprecated once the 0MQ version was ready, but it never was and noone seems to care about it so it's bitrotting atm.
If you place the bar too high so close to nobody encrypts their links, you end up with a totally cleartext network. And with stunnel etc the bar is probably already too high. |
That is not what I am claiming - my initial comment was very clear but I will try again. I am claiming that proper SSL protection cannot be done automatically by the person running the RPC server. Some user intervention is required, unless the implementation attempts "trust on first use" which has a tiny window for a MitM. This approach has less security than the CA approach, but it may not be obvious to the average user. Or maybe we determine it does not matter. But maintaining server SSL code, when epee needs to be refactored into cpp files or completely written, is a tough sell because there are solutions to this that are usable right now. And we already have a mechanism for steering people to these solutions.
Would a typical user who sees "RPC now has SSL" expect that caveat? Part of the goal of And as for the second part "if you decide to accept any cert" - that comment shows my entire point. The "easiest" mode to avoid this situation is getting a signed cert. If the RPC server operator went through that trouble, why can't they figure out an outside process for handling SSL?
It is in use by the mymonero backend already, and will be again for the open source edition. The switch occurred because there were some binary only endpoints that required the C++ deserialization code, wheres 0MQ used JSON for everything. So its certainly usable, and pub/sub is desirable in many cases as well. It also has the advantage of a built-in background thread for processing all comms, so the next round of blocks can be fetched by the 0MQ I/O thread, etc (although we could do this with a major epee re-write/factor). There are some other built-in modes for having multiple daemons service requests, etc., which can also be done with HTTP proxing. But is nice to just drop all of the overhead of HTTP ...
What is the goal of encrypting the P2P links? The data is ultimately public, and its difficult to hide that you are running a Monero node due to the majority of nodes using the standard ports + many inbound connections (if allowed). The one advantage I can think of is concealing the origin IP for a transaction - someone with lots of passive surveillance has an advantage. But transaction privacy is one of the primary uses for Kovri. Also you completely ignored the claim about SSL being a complicated protocol. Monero does tons of custom cryptographic operations already, and an "opportunistic encryption" scheme like BIP? needs : public key swapping, ECDH, keccack shared result, chacha20+poly1305. Monero does all of those things in various places throughout the code already, with the exception of poly1305. We should at least reach out to Bitcoin developers to see why they decided to not use SSL for this identical feature. |
Definitely need to know why. That BIP is next to useless - it mentions a couple MITM attack scenarios in its Motivation section but doesn't defend against any of them. What's the point of publishing a proposal that explicitly doesn't solve the problems it identifies? |
Right, sorry, I'd misread that as vs CA. But the "knowing the other side is really who you think it is" is not possible without doing anything in the first place AFAIK, whether this is specifying a key (or a set of keys) to trust, or asking a third party to decide for you. If you have a problem with the default to accept any cert, it can be changed for RPC (though for P2P I think it makes sense to accept any by default).
OK, I thought there was a pending patch by tewinget that was bitrotting. Is that optional then, or did you not need this part ?
Hiding traffic. Including whay you mentioned. If you say "you can still work something out this way, so we should do nothing yet", then I do not agree. As for Kovri, it's still not ready.
No. I agree with this. |
Not entirely. For RPC usage, you can build tunnels manually with the config file. For P2P, if monero had socks proxy support, you could use kovri socks proxy. |
What specific threat (to RPC) is this code is trying to mitigate? Simply to force the attacker to be active instead of passive? Or to prevent MitM? Obviously authentication and privacy are highly desirable in the RPC context. The target audience is a user whose node operator couldn't setup an SSL proxy for them. Does this target audience understand the risks? Its such a weird target audience (giving the total context). My next criticism is that this implementation is trust-on-connect. It likely needs to be trust-on-first-use for this target audience, to make the MitM attack more difficult (and therefore mitigating the risk for them even more), but that adds to the complexity of the implementation. EDIT: I wanted to make it clear that I am still against adding SSL to the RPC server until specific threat(s) and/or targeted users are identified. The current implementation is simply a response to the complaint that "RPC should have SSL" as if this can be done properly by magic. The only scenario that this seems to benefit is when a node operator is unable, unwilling, or unaware of the benefits to providing a SSL proxy. And the most value that the project can add is eliminating completely passive monitoring, but nothing more (because if the user can setup certs, they can setup a SSL proxy with better MitM prevention than trust-on-connect or a more complex to maintain trust-only-on-first-use).
This is why I've been pushing back against encryption for P2P communication - TLS cannot provide some magic solution against MitM in this situation either. We need to identify specific threat(s) that can be prevented by adding TLS, otherwise this is adding to the attack surface without benefit. One of the Bitcoin developers argued that an adversary would be unaware which links had known authentication keys, and therefore risked exposure if they attempted a MitM. This wouldn't necessarily help if a specific target (user) was chosen, and we still need to identify specific threat(s) to sending unencrypted data over the P2P links. I (and @moneromooo-monero over IRC) have come up with passively tracking the origin of a transaction or block as potential threats. Also, I realized that the IP peerlist is exchanged over P2P; a passive adversary not near either node can get a peek at the connection topology of those nodes (an adversary passively monitoring near either node will always be aware of the topology). Transaction and block origin attacks still have active techniques - which is why Kovri is on the roadmap. Peerlist exposure can always be exposed through an active attack or passive attack (but in less situations if encrypted). It is not immediately obvious that encrypting the Monero P2P links is worth the risk, given that we are still pursuing Kovri for the biggest metadata/information leaks. And OpenSSL has had issues specifically with the TLS code in the recent past. Pushing more of the attack surface to Kovri means that a really bad bug can take down the Kovri network, but not the Monero network. If the benefits of TLS outweigh the negatives, the project should still identify techniques to make the attack surface small as possible. My initial thoughts on this are: (1) require the newest TLS version only, (2) consider disabling heartbeats, (3) disable session resumption, (4) whitelist ciphers and public key methods. At this point, I'm still strongly in favor of letting Kovri do this for P2P data. The benefits are limited. Or the discussion needs to be more than just jam in "SSL support" as its too vague for the importance of the P2P communication channels.
The implementation works, but some of the serialization paths need some patching. Tewinget was trying to add "push" notifications to 0MQ through its pub/sub sockets for new blocks + mempool changes. Currently implementations have to poll over HTTP or 0MQ for this information. Push notifications over HTTP (long polling) is kind of crummy but possible. 0MQ always has less overhead to parse and serialize the protocol. |
There was no reply, save for some guy who was sarcastic without attack surface. |
This is a misrepresentation of what he said. He was not being sarcastic in his response. He was somewhat non-specific in his criticisms, primarily stating that he felt like SSL has a large attack surface. The only elaboration was on his experience with the X.509 format - he felt like his prior usage of that format was a "mistake". |
|
Whatever. I read "why would anyone want to use SSL" or whatever the exact words were, as sarcastic, maybe it was not. It's not the point anyway. I'll make another patch using that BIP system, but not sure when atm. And rereading what I wrote, I meant about attack surface above. |
|
Though this system can't really work for RPC connections, since the clients will be random other programs like curl, JSON RPC libraries... Which gets us back to the idea of using stunnel ending up with almost nobody bothering. |
|
Also related, I got this: ==7972==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62900004f258 at pc 0x58d94486f622 bp 0x77ef29aaaaf0 sp 0x77ef29aaa2a0 |
|
Or maybe I can make a SSH tunneling tool/script that's easy to use with monero... |
I don't understand. Do you mean many libraries will reject the connection for having unverifiable certificates?
Do you think if we provide a built-in solution for encryption/authentication, that this same user group will perform some kind of authentication (inspecting certificates or signing them)? The authentication part is important for these RPC channels too. This is what I'm doubting, that this same user group will go through with that effort. SSH proxies in particular are easy to setup (tons of online tutorials) and SSH mitigates poor certificate verification by storing it (trust on first use). GnuTLS also has a method for storing pubkeys, specifically for trust on first use. I'm not sure I like that though, should be application specific ... And another thing that influences my thinking on this topic - what do we expect the typical (or even lowest) user experience level for running a remote RPC connection? This isn't the node+GUI user, which needs to work properly even with low user experience levels.
|
I mean if we use the Bitcoin system, some of the RPC connections coming from things like curl or JSON RPC libraries will have to be modified to add this manually. As opposed to the daemon RPC coming from the wallet, which can be pre-modified to support this. It mean it makes the monero RPC really hard to use. And if this encryption is optional, it will again push people to use unencrypted links.
Why not ? If the default is to restrict to the pubkeys/certs given on the command line, it'd take work to do it wrong, no ? |
Or help with kovri development 😄 😉 |
|
I don't intend to spend months or years on this :P |
|
Sadly unwanted. |
|
I think this should go in after all. While we'll hopefully have Tor hidden service support for RPC, this still requires Tor to run, while this patch does not need any further tools. I fixed a use after free bug, might be the cause of the corruption above. I've not seen corruption again so far. I changed the default to disallow SSL connections if no allowed cert is specified on the command line. |
moneromooo-monero
force-pushed
the
ssl
branch
from
535c72d
to
91931cc
Compare
| X509_gmtime_adj(X509_get_notBefore(cert), 0); | ||
| X509_gmtime_adj(X509_get_notAfter(cert), 3600 * 24 * 365 * 10); // 10 years | ||
| X509_set_pubkey(cert, pkey); | ||
| X509_NAME *name = X509_get_subject_name(cert); |
There was a problem hiding this comment.
This cert was just created, so its subject_name is empty. Do we want to put some kind of non-empty name in here?
There was a problem hiding this comment.
That's a good question. Perhaps something with boost::asio::hostname() ?
There was a problem hiding this comment.
I would definitely not want my hostname to leak through a SSL certificate. What is that name used for in practice ?
There was a problem hiding this comment.
The name in a server cert usually corresponds to its hostname, and clients will verify that the name in the cert matches the name the client tried to connect to. This is one of the typical means of detecting spoofing attacks. Before progressing any further, we should define what is the threat that SSL is being used to protect against?
There was a problem hiding this comment.
AFAIK that name matching's only useful when using CAs. Here, we don't, we give monero the cert we expect to see. No spoofing possible (unless the cert's private key was compromised).
SSL is now used only for RPC. The point is to hide the data from network level snoopers (or people who'd modify the data on the way). And generally add some more encrypted traffic to piss off grab-everything snoops :)
For those people who use a round robin RPC server, they'll have to be set to accept any cert, but they're not expecting to connect to any particular server anyway, so nothing lost. They still gain privacy from their ISP (unless that ISP actively MITMs).
There was a problem hiding this comment.
we give monero the cert we expect to see.
I'm missing something here, then. This code generates a cert on the fly, on the monerod. So how does the client "expect" to see this? There's no protection against MITM at all, since any 3rd party could intercept and use their own generated cert. If active MITM is not something we're defending against, we can just skip all of this and use an anonymous Diffie-Hellman ciphersuite for the SSL session.
There was a problem hiding this comment.
It seems to be only used when certificate_path is empty, so when no certificate was generated, this code protects against passive eavesdropping, when given, active MITM.
There was a problem hiding this comment.
Yes, as above. Additionally, if you use --*-allow-any-cert, it will accept any. Otherwise it will require a cert that's in the --*-allowed-certificates list.
The idea is that a public RPC daemon nay establish a known identity (optional) and accept any cert, a private daemon that one wants to access through the internet will establish a know identity and require a particular cert, a wallet using a third party node will let a random cert be generated (avoids fingerprinting), and a wallet using a private node will have its own cert (which the private node will expect).
moneromooo-monero
force-pushed
the
ssl
branch
from
91931cc
to
d58bc29
Compare
| } | ||
|
|
||
| // detect SSL - we require all bytes to be there on the first recv for simplicity | ||
| if (is_ssl((const unsigned char*)buffer_.data(), bytes_transferred)) |
There was a problem hiding this comment.
As the comments indicate, this fails if the TLS message is split over TCP packets, so its broken.
There was a problem hiding this comment.
I had this queued up ages ago, and didn't delete it. Ignore for now.
There was a problem hiding this comment.
I intended to, but actually it's not too difficult to fix, though I can't seem to be able to test this, sending a char at a time with a second's delay still ends up with boost buffering all.
| m_ctx.set_default_verify_paths(); | ||
| m_ctx.context.set_options(boost::asio::ssl::context::default_workarounds | boost::asio::ssl::context::no_sslv2); | ||
| m_ctx.context.set_default_verify_paths(); | ||
| m_ssl_socket.~stream(); |
There was a problem hiding this comment.
Assign a default constructed object to m_ssl_socket. The current code might be implementation defined.
There was a problem hiding this comment.
Interesting. I have no idea why I did it that way...
Why is it implementation defined btw ?
And you meant the ctor call below, right, not default ?
There was a problem hiding this comment.
Ah. I found why:
/home/user/src/bitmonero/contrib/epee/include/net/net_helper.h:140:18: error: object of type 'boost::asio::ssl::stream<boost::asio::basic_stream_socket<boost::asio::ip::tcp, boost::asio::stream_socket_serviceboost::asio::ip::tcp > >' cannot be assigned because its copy assignment operator is implicitly deleted
There was a problem hiding this comment.
If either the destructor or constructor throws, m_ssl_socket is in an indeterminate state and UB will occur the next time the object is touched (during destruction, for instance). It's fairly tricky to code around this - this is yet-another sneakily weird edge case within the language.
I know something similar is done in epee, but its something that should likely be updated too.
There was a problem hiding this comment.
It seems like there has to be a better way to reset state of the object, if not boost::optional will do what you want. Or you'll manually have to use std::aligned_storage and use some bool to track state.
There was a problem hiding this comment.
That all sounds very unlikely, no ?
There was a problem hiding this comment.
The documentation does not list any possible exceptions, however neither function is marked as noexcept. I'm not sure about the ASIO developer, but several Boost developers view noexcept as a permanent API specification. Therefore the lack of noexcept means some future release could throw. And I'm not entirely certain that these can't throw now; the constructor and destructor sequences are platform specific (due to internal boost::asio::ip::tcp::socket).
If there is no other good technique for resetting state (there doesn't appear to be), UB can be removed with heap allocation, boost::optional, or a custom std::aligned_storage. Leaving the potential of UB doesn't seem worth it considering the fixes are relatively easy, and two of them can avoid additional allocations.
There was a problem hiding this comment.
OK, I'll add a boost::optional here then.
There was a problem hiding this comment.
Actually, a shared_ptr seems even better AFAICT. I'll do that and see if anything complains.
There was a problem hiding this comment.
Ah, nevermind, that's what you said via heap allocation above. So I'll do that.
| @@ -110,25 +111,34 @@ namespace net_utils | |||
| catch(...) { /* ignore */ } | |||
| } | |||
|
|
|||
| inline void set_ssl(epee::net_utils::ssl_support_t ssl_support = epee::net_utils::e_ssl_support_autodetect, const std::string &private_key = std::string(), const std::string &certificate = std::string(), const std::list<std::string> &allowed_certificates = std::list<std::string>(), bool allow_any_cert = false) | |||
There was a problem hiding this comment.
I have simultaneously been working on updating client SSL stuff here since the proxy code has to touch this very same portion of code. Then I realized that this PR almost certainly changed this function (yup).
I started to use a boost::variant to specify the parameters. This allowed for specifying the behavior and values in the same object. For instance, setting the last parameter to true (in this patch) ignores several of the other parameters entirely which is weird. The code I was working with looks something like:
#include <boost/variant/variant_fwd.hpp>
namespace ssl
{
struct none_
{};
constexpr const none_ none{};
struct any_certificate_
{};
constexpr const any_certificate_ any_certificate{};
struct default_certificates_
{};
constexpr const default_certificates_ default_certificates{};
struct certificate_file
{
std::string path;
};
using mode = boost::variant<none_, any_certificate_, default_certificates_, certificate_file>;
}
// client
#include <boost/variant/variant.hpp>
m_http_client.connect(address, port, timeout, ssl::default_certificates);
m_http_client.connect(address, port, timeout, ssl::certificate_file{"/etc/server/file.pem"});Also, if multiple certs are removed then it becomes much easier because there is built-in functionality with asio::ssl/openssl to use a single cert. Since one certificate can sign multiple server certificates, when would this functionality be used? It seems easier to omit the logic (and thus any potential bugs/memory leaks).
There was a problem hiding this comment.
OK, seems fair enough. The param list is a bit unwieldy indeed.
If I remove the multiple cert thing, then not much code is saved, and it forces the user to select the right cert for every run if they usually use more than one peer. Given it's C++ containers, I really do not expects leaks either.
There was a problem hiding this comment.
I meant the usage of openssl calls. create_ssl_certificate should need some frees at the end, and there are a number of places where a std::string constructor can throw and leak memory. The latter scenario is unlikely at least.
There was a problem hiding this comment.
Sure, but using this as an argument for supporting only one allowed certificate seems reaching quite a bit. If there are leaks on error paths, let's fix them.
There was a problem hiding this comment.
I looked up the documentation for SSL_CTX_load_verify_locations, and multiple certificates can be put into a single file. ASIO doesn't forward to this function, it would be have to be called manually. Seems easier than risking bugs in iterating through a list manually.
There was a problem hiding this comment.
What code are you suggesting replacing ? The only code I see looping through allowed certificates is:
std::list ssl_allowed_certificates;
for (const std::string &path: daemon_ssl_allowed_certificates)
{
ssl_allowed_certificates.push_back({});
if (!epee::file_io_utils::load_file_to_string(path, ssl_allowed_certificates.back()))
{
MERROR("Failed to load certificate: " << path);
ssl_allowed_certificates.back() = std::string();
}
}
but that doesn't quite seem right to me.
moneromooo-monero
force-pushed
the
ssl
branch
from
d58bc29
to
cf298a1
Compare
|
If you know which cipher specs are good to whitelist, go ahead. Here, they're all reported as strong by nmap, but there's still more than a dozen, and I'm not sure whether it's machine dependent or not. |
moneromooo-monero
force-pushed
the
ssl
branch
2 times, most recently
from
3582b16
to
fd0dc8e
Compare
|
I had chosen these ciphers in my PR: ECDHE-ECDSA-CHACHA20-POLY1305-SHA256:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES256-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-CHACHA20-POLY1305 This gives the best security and performance - especially if both sides support TLS 1.3. It is, however, also compatible with TLS 1.2 (with slightly reduced performance). |
|
OK, if you added whitelisting already, I'll just leave it as is and we'll use your PR after this one is merged. Thanks. |
|
No further comments ? |
contrib/epee/src/net_ssl.cpp
Outdated
| << (unsigned)(unsigned char)data[4] << " " << (unsigned)(unsigned char)data[5] << " " | ||
| << (unsigned)(unsigned char)data[6] << " " << (unsigned)(unsigned char)data[7] << " " | ||
| << (unsigned)(unsigned char)data[8]); | ||
| if (len >= 9) |
There was a problem hiding this comment.
This is a duplicate check? And its after derefencing in the above log statement.
Also, get_ssl_magic_size() can be constexpr so that a static_assert be added to ensure the check is valid for those dereferenced bytes.
| if (data[0] == 0x16) // record | ||
| if (data[1] == 3) // major version | ||
| if (data[5] == 1) // ClientHello | ||
| if (data[6] == 0 && data[3]*256 + data[4] == data[7]*256 + data[8] + 4) // length check |
There was a problem hiding this comment.
This arithmetic is done as an int per C/C++ rules. They could overflow (although it would be rare - it would be a platform where int is 16-bits). I would wrap one of the values on each side of the == into a std::uint32_t to bump the expression arithmetic to a minimum of 32-bits.
There was a problem hiding this comment.
It should be enough to use 256u here, right, if we wanted to support 16 bit ints ?
There was a problem hiding this comment.
Yeah, I definitely did the math on this wrong during the initial review. The max value here should be 65535 (255*256 + 255 = 65535).
|
|
||
| bool is_ssl(const unsigned char *data, size_t len) | ||
| { | ||
| if (len < get_ssl_magic_size()) |
There was a problem hiding this comment.
This function should return 3 possible values: (1) is ssl, (2) is not ssl, or (3) not enough data.
There was a problem hiding this comment.
I'm happy with false in this case. I could add an exception if below magic size if you'd rather.
| } | ||
| std::string certificate(std::string(buf->data, buf->length)); | ||
| BIO_free(bio_cert); | ||
| return std::find(allowed_certificates.begin(), allowed_certificates.end(), certificate) != allowed_certificates.end(); |
There was a problem hiding this comment.
My earlier comment about looping through certificates manually was about this function. It has memory leaks in a few pessimistic cases, and can be replaced with SSL_CTX_load_verify_locations (mentioned previously). I used that function in my Socks+RPC patch.
The OpenSSL function takes a single file argument, but the file can have multiple certificates within one file. Additionally, the function doesn't do an "exact" match (as done here), but instead treats the certificates in the file as CAs so that verifying one certificate can be used to authorize multiple sites. There is no practical difference in security - in either scenario if a key is compromised that key + "root" key must be tossed since revocation is shaky. If allowed_certificates were "pinned" to a hostname - making them valid in a specific context only - then this function would differ a bit from the built-in function. And even then, a callback can be added to do further restrictions on the built-in function.
There was a problem hiding this comment.
If you're refering to std::string ctor throwing out of memory, I can make the bio ptr a unique_ptr. I'd rather not change the SSL code now, but if you're sure this does the same thing, you can change it later. omartinj changed this so you can whitelist fingerprints as well as certificates too.
There was a problem hiding this comment.
I've used the code you suggested below.
contrib/epee/src/net_ssl.cpp
Outdated
| bool verified = false; | ||
| socket.next_layer().set_option(boost::asio::ip::tcp::no_delay(true)); | ||
| socket.set_verify_mode(boost::asio::ssl::verify_peer); | ||
| socket.set_verify_depth(1); |
There was a problem hiding this comment.
I'm not 100% sure now. I think it might have said a cert was verified if it was signed by a root CA or something. In any case, since I don't care about signing with other certs, a depth of 1 ensures that such signing isn't taken into account in the first place AFAICT.
There was a problem hiding this comment.
The depth setting is related but separate from "root" CAs on a system - they require an explicit call to load. This set_verify_depth calls prevents anyone from using a certificate that can sign other certificates. Why prevent this capability?
There was a problem hiding this comment.
Are you sure ? The manpage says "sets the maximum depth for the certificate chain verification that shall be allowed". I don't read this as what you say here.
There was a problem hiding this comment.
Setting verify_depth to 1 means you will only allow self-signed certs to be used. This doesn't sound like a good idea. It prevents using any cert that was signed by any other cert. Best practice with certs is to generate a single signing cert and use it to sign server/client certs for all the endpoints that need them.
My mistake: depth=1 means only certs signed by a root CA (self-signed cert) are allowed. So this prohibits using certs signed by intermediate CAs.
There was a problem hiding this comment.
Ah, I see, that makes sense, I'll remove it.
| return false; | ||
| } | ||
| cert_buffer = std::string(buf->data, buf->length); | ||
| return success; |
There was a problem hiding this comment.
Two missing BIO_frees here that would be automatic with std::unique_ptr usage.
contrib/epee/src/net_ssl.cpp
Outdated
| if (!rsa) | ||
| { | ||
| MERROR("Error generating RSA private key"); | ||
| EVP_PKEY_free(pkey); |
There was a problem hiding this comment.
Leaks if log statement throws. Consider:
struct openssl_public_free
{
void operator()(EVP_PKEY* ptr) const noexcept
{
if (ptr)
EVP_PKEY_free(ptr);
}
};
using openssl_public = std::unique_ptr<EVP_PKEY, openssl_public_free>;
struct openssl_x509_free
{
void operator()(X509* ptr) const noexcept
{
if (ptr)
EVP_PKEY_free(ptr);
}
};
using openssl_x509 = std::unique_ptr<EVP_PKEY, openssl_x509_free>;
contrib/epee/src/net_ssl.cpp
Outdated
| } | ||
| ASN1_INTEGER_set(X509_get_serialNumber(cert), 1); | ||
| X509_gmtime_adj(X509_get_notBefore(cert), 0); | ||
| X509_gmtime_adj(X509_get_notAfter(cert), 3600 * 24 * 365 * 10); // 10 years |
There was a problem hiding this comment.
Is this the expiration date? 10 years seems too long for a temporary key. I wouldn't make this more than a year, and perhaps as low as 6 months.
src/wallet/wallet2.cpp
Outdated
| auto daemon_ssl_allow_any_cert = command_line::get_arg(vm, opts.daemon_ssl_allow_any_cert); | ||
| auto daemon_ssl = command_line::get_arg(vm, opts.daemon_ssl); | ||
| epee::net_utils::ssl_support_t ssl_support; | ||
| if (daemon_ssl == "enabled") |
There was a problem hiding this comment.
Seen this conversion from string twice, seems an easy function to make to keep consistent.
| @@ -198,8 +203,8 @@ namespace net_utils | |||
| std::map<std::string, t_connection_type> server_type_map; | |||
| void create_server_type_map(); | |||
|
|
|||
| bool init_server(uint32_t port, const std::string address = "0.0.0.0"); | |||
| bool init_server(const std::string port, const std::string& address = "0.0.0.0"); | |||
| bool init_server(uint32_t port, const std::string address = "0.0.0.0", epee::net_utils::ssl_support_t ssl_support = epee::net_utils::e_ssl_support_autodetect, const std::string &private_key_path = std::string(), const std::string &certificate_path = std::string(), const std::list<std::string> &allowed_certificates = {}, bool allow_any_cert = false); | |||
There was a problem hiding this comment.
The private key and public certificate should likely be combined into one struct argument to make the relationship clear (they must come in pairs).
moneromooo-monero
force-pushed
the
ssl
branch
3 times, most recently
from
5140fcb
to
32dd8dc
Compare
|
Now rebased onto master. |
|
I've moved the code to separate functions. It's not quite as large a diff as I thought. I'll squash once you've read it. |
moneromooo-monero
force-pushed
the
ssl
branch
from
64058af
to
3f56c70
Compare
| sock_.shutdown(boost::asio::ip::tcp::socket::shutdown_both, ignored_ec); | ||
| sock_.close(); | ||
| ssl = false; | ||
| return true; |
There was a problem hiding this comment.
This comment is minor and applies to the bottom (other) case too.
I expected this to return false (the connection failed), and then let the caller determine what to do next. That way, the closing behavior (the portion right here) could be used for either autodetect or enabled mode (which has a separate close routine just below this), and the ssl parameter would not need to be passed because the caller would know the context. I.e. this function just fails and the caller determines whether it should run again; returning true here makes the ssl parameter necessary.
There was a problem hiding this comment.
You need both (3 cases can happen): the physical connection can succeed, but if the peer does not support SSL, the logical connection fails. Then you know you should retry without SSL, which is that case: return true because the connection succeeds, but set ssl to true to say "but we actually need SSL". I suppose you'd use a boost::optional for that :)
There was a problem hiding this comment.
Yes, forgot about the case of connection failure with SSL enabled (differed from connection success with SSL failure). In this instance, I would probably use an enum.
There was a problem hiding this comment.
Well, I might have advocated for a boost::optional<bool> in the past, but the difference between null, true, and false seems kind of funky here.
There was a problem hiding this comment.
I'll make it an enum then.
RPC connections now have optional tranparent SSL.
An optional private key and certificate file can be passed,
using the --{rpc,daemon}-ssl-private-key and
--{rpc,daemon}-ssl-certificate options. Those have as
argument a path to a PEM format private private key and
certificate, respectively.
If not given, a temporary self signed certificate will be used.
SSL can be enabled or disabled using --{rpc}-ssl, which
accepts autodetect (default), disabled or enabled.
Access can be restricted to particular certificates using the
--rpc-ssl-allowed-certificates, which takes a list of
paths to PEM encoded certificates. This can allow a wallet to
connect to only the daemon they think they're connected to,
by forcing SSL and listing the paths to the known good
certificates.
To generate long term certificates:
openssl genrsa -out /tmp/KEY 4096
openssl req -new -key /tmp/KEY -out /tmp/REQ
openssl x509 -req -days 999999 -sha256 -in /tmp/REQ -signkey /tmp/KEY -out /tmp/CERT
/tmp/KEY is the private key, and /tmp/CERT is the certificate,
both in PEM format. /tmp/REQ can be removed. Adjust the last
command to set expiration date, etc, as needed. It doesn't
make a whole lot of sense for monero anyway, since most servers
will run with one time temporary self signed certificates anyway.
SSL support is transparent, so all communication is done on the
existing ports, with SSL autodetection. This means you can start
using an SSL daemon now, but you should not enforce SSL yet or
nothing will talk to you.
moneromooo-monero
force-pushed
the
ssl
branch
from
3f56c70
to
2456945
Compare
2456945 epee: add SSL support (moneromooo-monero)
P2P and RPC connections now have optional tranparent SSL.
An optional private key and certificate file can be passed,
using the --{p2p,rpc,daemon}-ssl-private-key and
--{p2p,rpc,daemon}-ssl-certificate options. Those have as
argument a path to a PEM format private private key and
certificate, respectively.
If not given, a temporary self signed certificate will be used.
SSL can be enabled or disabled using --{p2p,rpc}-ssl, which
accepts autodetect (default), disabled or enabled.
Access can be restricted to particular certificates using the
--{p2p,rpc}-ssl-allowed-certificates, which takes a list of
paths to PEM encoded certificates. This can allow a wallet to
connect to only the daemon they think they're connected to,
by forcing SSL and listing the paths to the known good
certificates.
To generate long term certificates:
openssl genrsa -out /tmp/KEY 4096
openssl req -new -key /tmp/KEY -out /tmp/REQ
openssl x509 -req -days 999999 -sha256 -in /tmp/REQ -signkey /tmp/KEY -out /tmp/CERT
/tmp/KEY is the private key, and /tmp/CERT is the certificate,
both in PEM format. /tmp/REQ can be removed. Adjust the last
command to set expiration date, etc, as needed. It doesn't
make a whole lot of sense for monero anyway, since most servers
will run with one time temporary self signed certificates anyway.
SSL support is transparent, so all communication is done on the
existing ports, with SSL autodetection. This means you can start
using an SSL daemon now, but you should not enforce SSL yet or
nothing will talk to you.