Skip to content

Commit

Permalink
Allow multiple keypairs to be specified for crypto agility
Browse files Browse the repository at this point in the history
  • Loading branch information
@arr2036
arr2036 committed Mar 13, 2018
1 parent 883f0c5 commit e8df16d
Show file tree
Hide file tree
Showing 8 changed files with 190 additions and 114 deletions.
42 changes: 24 additions & 18 deletions raddb/mods-available/eap
View file
Expand Up @@ -163,26 +163,32 @@ eap {
# you want.
#
tls-config tls-common {
private_key_password = whatever
private_key_file = ${certdir}/server.pem

#
# If Private key & Certificate are located in the same file,
# then private_key_file & certificate_file must contain the
# same file name.
#
# If ca_file (below) is not used, then the certificate_file
# below MUST include not only the server certificate, but ALSO
# all of the CA certificates used to sign the server
# certificate.
#
# Any certificate chain MUST be in order from server
# certificate (first in the file) to intermediaries (second) to
# root CA (last in the file) as per RFC 4346 (see
# certificate_list
# http://tools.ietf.org/html/rfc4346#section-7.4.2 )
# Multiple certificate sections may be specified to support
# crypto agility with different key types.
#
certificate_file = ${certdir}/server.pem
certificate {
private_key_password = whatever
private_key_file = ${certdir}/server.key

#
# If Private key & Certificate are located in the same file,
# then private_key_file & certificate_file must contain the
# same file name.
#
# If ca_file (below) is not used, then the certificate_file
# below MUST include not only the server certificate, but ALSO
# all of the CA certificates used to sign the server

This comment has been minimized.

Sign in to view

Copy link
@restena-sw

restena-sw Mar 14, 2018

Contributor

It really SHOULD NOT contain the root CA though. Only intermediates. If the root is in the file, it will be sent in the EAP exchange which is superfluous (the client has authoritative info which root to trust, and has a copy - if it doesn't, it also doesn't add security to "helpfully" deliver a trust root by the one who wants to be trusted).
This is also in big red letters (well... imaginary, since RFC doesn't have any markup ;-) in the EAP-TLS 1.3 RFC. And that advice has been valid ever since, not just with the introduction of 1.3.

This comment has been minimized.

Sign in to view

Copy link
@arr2036

arr2036 Mar 14, 2018

Author Member

The issue appears to be that the certificate store of trusted CAs is used both for validating client certificates and for building up local certificate chains.

In the case where the server's cert was signed by a different CA to the one that signed the client certificates (very common), we'd not want to trust client certs signed by the server CA, and so would need to include it in the server.pem file... That said I don't know if OpenSSL actually requires a complete certificate chain for the server.

This comment has been minimized.

Sign in to view

Copy link
@restena-sw

restena-sw Mar 14, 2018

Contributor

But then we're talking about a regression? 3.0.x explicitly allowed to stop the amalgamation of the cert store for both client and server. Changelog of 3.0.11:

    * Add "auto_chain" to raddb/mods-available/eap, tls
      subsection.  This allows the disabling of OpenSSL
      auto-chaining of certificates.  Which might be wrong.

In fact we'd asked for that for eduroam. The issue is that if your client certs and server cert come from the same CA, FreeRADIUS would send the entire chain with root cert during the EAP exchange. When it really shouldn't as this root CA is a useless inflation of the packet exchange.

Basically, ca_file and ca_dir are fine to verify client certs against. But for the server cert, FR should take what's in the cert_file and send it out. No unhelpful auto-complete with root CAs please.

OpenSSL does NOT require a chain - it takes the certs it gets from input and sends them.

This is a quite common setup in eduroam. Our tooling actively tests and warns the admin if the root cert is being sent erroneously.

Also, the EAP-TLS 1.3 doc makes the exact same advice:

"Endpoints SHOULD reduce the sizes of Certificate messages by omitting
certificates that the other endpoint is known to possess. When using
TLS 1.3, all certificates that specifies a trust anchor may be
omitted (see Section 4.4.2 of [I-D.ietf-tls-tls13]). When using TLS
1.2 or earlier, only the self-signed certificate that specifies the
root certificate authority may be omitted (see Section 7.4.2 of
[RFC5246])."

Could you confirm that auto_chain is still functional?

This comment has been minimized.

Sign in to view

Copy link
@arr2036

arr2036 Mar 14, 2018

Author Member
  • I didn't write the text about which certificates should be included in the server.pem file, I just indented it. It confused me too, I couldn't see any reason why you'd have to complete the chain on the server side unless it was some undocumented OpenSSL requirement.
  • auto_chain = yes/no is still present and functional, but I'm strongly tempted to switch the default to no for v4 as it seems to be the most desirable behaviour, and the easiest to explain. Thoughts?
  • ca_dir would in theory have no effect on client certificate validation, unless the client ignores the list of acceptable CAs (populated from ca_file). Do they do this in the real world? Is the list just a hint?
  • I'm going to document the outcome of this discussion in mods-available/eap as the documentation around the specifics of how FreeRADIUS/OpenSSL functions is definitely lacking.

This comment has been minimized.

Sign in to view

Copy link
@restena-sw

restena-sw Mar 14, 2018

Contributor

He, great discussion on one single /comment/ LOC :-)

I also see no point in auto_chain = yes, so all good for making it a No, but am not confident enough some configs might implicitly rely on this and break. If the changelog documents the new default, that should be okay.

You are confusing me a lot with the bullet on ca_file and ca_dir. My understanding since a decade-long while is that both have the same effect, you can set either one of the two depending on what's your preference for storing the CA cert(s):
if you have a really small list, you can stuff them all into one file and reference in ca_file (but you have a hard scripting time updating CRLs if there is more than one)
Alternatively, you put the CAs one by one PEM file (and their CRLs if any) into a directory and reference it as ca_dir. OpenSSL only picks up those CAs if you've also run the "c_rehash ." command in that directory so that they can be looked up by their hash. That last bit is probably the most common point of failure; people forget c_rehash and then no CA is recognised.

To be honest I have no idea what happens if both _file and _dir are set. I always considered this a misconfiguration. If the code then gives one precedence over the other, this should also be documented.

This comment has been minimized.

Sign in to view

Copy link
@alanbuxey

alanbuxey via email Mar 14, 2018

Contributor

This comment has been minimized.

Sign in to view

Copy link
@arr2036

arr2036 Mar 14, 2018 •
edited

Author Member
  • ca_dir operates as you say, the bit that's not obvious is that we call SSL_CTX_set_client_CA_list(ctx, SSL_load_client_CA_file(conf->ca_file)); if ca_file is set. I'm not sure what happens if we don't do that... could be that all the CAs in the ca_dir are added implicitly to the list of advertised CAs.
  • As of OpenSSL 1.0.2 there are now completely independent verification and certificate chain stores (though the combined store is still used by default). The verification store contains the certificates used to verify the peer's certificate, the chain store contains the certificate chain(s) presented to the peer.
  • I've seen comments online that OpenSSL 1.0.2 is required to manage the multiple chains required for crypto agility so if you're testing you may want to upgrade to that. They are really completely independent certificate chains, one maintained for each key type e.g. RSA, DSA, ECC etc..
  • As of OpenSSL 1.0.2 there are functions available to explicitly build certificate chains on startup, and to validate those chains. There's also a toggle to include the root CA or not when building the chains.

This comment has been minimized.

Sign in to view

Copy link
@restena-sw

restena-sw Mar 15, 2018

Contributor

What Alan said :-)

  • cert_file -> this contains what the server is going to present to the client
  • ca_file, ca_dir -> this contains the trust anchors for certs the client presents during EAP-TLS (or PEAP and TTLS with optional client certificates, FWIW)

If client certs are not used, ca_file and ca_dir are useless and should be commented out.

(before auto_chain, not setting either of the ca_ options was the thing to do to prevent sending the root; however if you did EAP-TLS and your server cert happened to be from the same CA as the client certs you expect to come in, that was not an option and it was impossible to omit the root. That's where auto_chain = no came in handy)

This comment has been minimized.

Sign in to view

Copy link
@alandekok

alandekok Mar 15, 2018

Member

TBH, we could rename the configuration to just do:

  • server_certificate = /path/to/cert
  • ca_certificate = /path/to/ca

And if the ca_certificate path is a directory, assume ca_dir. Otherwise, it's like ca_file

That also allows for people putting certificates inline. i.e.

server_certificate = "omfg huge blob of ASCII crap"

I'm generally in favor of keeping it simple for the user, and having the server figure out the right thing to do.

This comment has been minimized.

Sign in to view

Copy link
@arr2036

arr2036 Mar 15, 2018 •
edited

Author Member

@alandekok yes for ca_dir vs ca_file, I honestly don't see a situation where configuring them both is useful, and it seems quite friendly to use the result of a stat to figure out which we need to use.

Not sure I see the benefit of static inline server certificates though, to me that could make the configuration significantly less readable, especially as the OpenSSL >= 1.0.2 behaviour requires the complete certificate chain to be provided.

It's also a fair amount of work to emulate what's done in SSL_CTX_use_certificate_chain_file as there is no BIO equivalent.

This comment has been minimized.

Sign in to view

Copy link
@arr2036

arr2036 Mar 15, 2018

Author Member

Ok, i'm done making changes. I've confirmed OpenSSL 1.0.2 allows multiple chains to be loaded, that chain precompilation works, and that chain precompilation does not use certs from ca_file or ca_path irrespective of the auto_chain value.

This comment has been minimized.

Sign in to view

Copy link
@restena-sw

restena-sw Mar 16, 2018

Contributor

Thanks a lot! Might be a while until I can take a look at 4.x but I'll note that this is something to try!
# certificate.
#
# Any certificate chain MUST be in order from server
# certificate (first in the file) to intermediaries (second) to
# root CA (last in the file) as per RFC 4346 (see
# certificate_list
# http://tools.ietf.org/html/rfc4346#section-7.4.2 )
#
certificate_file = ${certdir}/server.pem
}

#
# Server certificate may also be specified at runtime on a per
Expand Down
12 changes: 9 additions & 3 deletions raddb/mods-available/eap_inner
View file
Expand Up @@ -64,9 +64,15 @@ eap inner-eap {
# In the case of PEAP it also allows SoH with client certificates.
#
tls-config tls-peer {
private_key_password = whatever
private_key_file = ${certdir}/server.key
certificate_file = ${certdir}/server.pem
#
# Multiple certificate sections may be specified to support
# crypto agility with different key types.
#
certificate {
private_key_password = whatever
private_key_file = ${certdir}/server.key
certificate_file = ${certdir}/server.pem
}

#
# Trusted Root CA list
Expand Down
11 changes: 7 additions & 4 deletions raddb/sites-available/abfab-tls
View file
Expand Up @@ -10,11 +10,14 @@ listen {
proto = tcp

tls {
private_key_password = whatever

# Moonshot tends to distribute certs separate from keys
private_key_file = ${certdir}/server.key
certificate_file = ${certdir}/server.pem
certificate {
private_key_file = ${certdir}/server.key
certificate_file = ${certdir}/server.pem

private_key_password = whatever
}

ca_file = ${cadir}/ca.pem
dh_file = ${certdir}/dh
fragment_size = 8192
Expand Down
18 changes: 14 additions & 4 deletions src/include/tls-h
View file
Expand Up @@ -213,6 +213,18 @@ typedef struct {
} fr_tls_ocsp_conf_t;
#endif

/** Structure representing a single certificate
*
*/
typedef struct {
bool pem_file_type; //!< Whether the file is expected to be PEM encoded.
///< This allows us to load multiple chained PEM certificates
///< from a single file.
char const *password; //!< Password to decrypt the certificate(s).
char const *certificate_file; //!< Path to certificate.
char const *private_key_file; //!< Path to certificate.
} fr_tls_conf_key_pair_t;

/* configured values goes right here */
struct fr_tls_conf_t {
SSL_CTX **ctx; //!< We use an array of contexts to reduce contention.
Expand All @@ -223,9 +235,8 @@ struct fr_tls_conf_t {

CONF_SECTION *cs;

char const *private_key_password; //!< Password to decrypt the private key.
char const *private_key_file; //!< Private key file.
char const *certificate_file; //!< Public (certificate) file.
fr_tls_conf_key_pair_t **key_pairs; //!< One or more certificates

char const *random_file; //!< If set, we read 10K of data (or the complete file)
//!< and use it to seed OpenSSL's PRNG.
char const *ca_path;
Expand All @@ -236,7 +247,6 @@ struct fr_tls_conf_t {
uint32_t verify_depth; //!< Maximum number of certificates we can traverse
//!< when attempting to reach the presented certificate
//!< from our Root CA.
bool file_type;
bool auto_chain; //!< Allow OpenSSL to build certificate chains
//!< from all certificates it has available.
//!< If false, the complete chain must be provided in
Expand Down
81 changes: 54 additions & 27 deletions src/lib/tls/conf.c
View file
Expand Up @@ -80,14 +80,24 @@ static CONF_PARSER ocsp_config[] = {
};
#endif

CONF_PARSER tls_cert_pair_config[] = {
{ FR_CONF_OFFSET("pem_file_type", FR_TYPE_BOOL, fr_tls_conf_key_pair_t, pem_file_type), .dflt = "yes" },
{ FR_CONF_OFFSET("certificate_file", FR_TYPE_FILE_INPUT | FR_TYPE_REQUIRED , fr_tls_conf_key_pair_t, certificate_file) },
{ FR_CONF_OFFSET("private_key_password", FR_TYPE_STRING | FR_TYPE_SECRET, fr_tls_conf_key_pair_t, password) },
{ FR_CONF_OFFSET("private_key_file", FR_TYPE_FILE_INPUT | FR_TYPE_REQUIRED, fr_tls_conf_key_pair_t, private_key_file) },

CONF_PARSER_TERMINATOR
};

CONF_PARSER tls_server_config[] = {
{ FR_CONF_DEPRECATED("pem_file_type", FR_TYPE_BOOL, fr_tls_conf_t, NULL) },
{ FR_CONF_DEPRECATED("certificate_file", FR_TYPE_FILE_INPUT, fr_tls_conf_t, NULL) },
{ FR_CONF_DEPRECATED("private_key_password", FR_TYPE_STRING | FR_TYPE_SECRET, fr_tls_conf_t, NULL) },
{ FR_CONF_DEPRECATED("private_key_file", FR_TYPE_FILE_INPUT, fr_tls_conf_t, NULL) },

{ FR_CONF_OFFSET("verify_depth", FR_TYPE_UINT32, fr_tls_conf_t, verify_depth), .dflt = "0" },
{ FR_CONF_OFFSET("ca_path", FR_TYPE_FILE_INPUT, fr_tls_conf_t, ca_path) },
{ FR_CONF_OFFSET("pem_file_type", FR_TYPE_BOOL, fr_tls_conf_t, file_type), .dflt = "yes" },
{ FR_CONF_OFFSET("private_key_file", FR_TYPE_FILE_INPUT, fr_tls_conf_t, private_key_file) },
{ FR_CONF_OFFSET("certificate_file", FR_TYPE_FILE_INPUT, fr_tls_conf_t, certificate_file) },
{ FR_CONF_OFFSET("ca_file", FR_TYPE_FILE_INPUT, fr_tls_conf_t, ca_file) },
{ FR_CONF_OFFSET("private_key_password", FR_TYPE_STRING | FR_TYPE_SECRET, fr_tls_conf_t, private_key_password) },
#ifdef PSK_MAX_IDENTITY_LEN
{ FR_CONF_OFFSET("psk_identity", FR_TYPE_STRING, fr_tls_conf_t, psk_identity) },
{ FR_CONF_OFFSET("psk_hexphrase", FR_TYPE_STRING | FR_TYPE_SECRET, fr_tls_conf_t, psk_password) },
Expand Down Expand Up @@ -117,11 +127,14 @@ CONF_PARSER tls_server_config[] = {
{ FR_CONF_OFFSET("ecdh_curve", FR_TYPE_STRING, fr_tls_conf_t, ecdh_curve), .dflt = "prime256v1" },
#endif
#endif

{ FR_CONF_OFFSET("tls_max_version", FR_TYPE_FLOAT32, fr_tls_conf_t, tls_max_version) },

{ FR_CONF_OFFSET("tls_min_version", FR_TYPE_FLOAT32, fr_tls_conf_t, tls_min_version), .dflt = "1.0" },

{ FR_CONF_OFFSET("certificate", FR_TYPE_SUBSECTION | FR_TYPE_MULTI, fr_tls_conf_t, key_pairs),
.subcs_size = sizeof(fr_tls_conf_key_pair_t), .subcs_type = "fr_tls_conf_key_pair_t",
.subcs = tls_cert_pair_config },

{ FR_CONF_POINTER("cache", FR_TYPE_SUBSECTION, NULL), .subcs = (void const *) cache_config },

{ FR_CONF_POINTER("verify", FR_TYPE_SUBSECTION, NULL), .subcs = (void const *) verify_config },
Expand All @@ -135,13 +148,15 @@ CONF_PARSER tls_server_config[] = {
};

CONF_PARSER tls_client_config[] = {
{ FR_CONF_DEPRECATED("pem_file_type", FR_TYPE_BOOL, fr_tls_conf_t, NULL) },
{ FR_CONF_DEPRECATED("certificate_file", FR_TYPE_FILE_INPUT, fr_tls_conf_t, NULL) },
{ FR_CONF_DEPRECATED("private_key_password", FR_TYPE_STRING | FR_TYPE_SECRET, fr_tls_conf_t, NULL) },
{ FR_CONF_DEPRECATED("private_key_file", FR_TYPE_FILE_INPUT, fr_tls_conf_t, NULL) },

{ FR_CONF_OFFSET("verify_depth", FR_TYPE_UINT32, fr_tls_conf_t, verify_depth), .dflt = "0" },
{ FR_CONF_OFFSET("ca_path", FR_TYPE_FILE_INPUT, fr_tls_conf_t, ca_path) },
{ FR_CONF_OFFSET("pem_file_type", FR_TYPE_BOOL, fr_tls_conf_t, file_type), .dflt = "yes" },
{ FR_CONF_OFFSET("private_key_file", FR_TYPE_FILE_INPUT, fr_tls_conf_t, private_key_file) },
{ FR_CONF_OFFSET("certificate_file", FR_TYPE_FILE_INPUT, fr_tls_conf_t, certificate_file) },

{ FR_CONF_OFFSET("ca_file", FR_TYPE_FILE_INPUT, fr_tls_conf_t, ca_file) },
{ FR_CONF_OFFSET("private_key_password", FR_TYPE_STRING | FR_TYPE_SECRET, fr_tls_conf_t, private_key_password) },
{ FR_CONF_OFFSET("dh_file", FR_TYPE_STRING, fr_tls_conf_t, dh_file) },
{ FR_CONF_OFFSET("random_file", FR_TYPE_STRING, fr_tls_conf_t, random_file) },
{ FR_CONF_OFFSET("fragment_size", FR_TYPE_UINT32, fr_tls_conf_t, fragment_size), .dflt = "1024" },
Expand All @@ -160,46 +175,57 @@ CONF_PARSER tls_client_config[] = {

{ FR_CONF_OFFSET("tls_min_version", FR_TYPE_FLOAT32, fr_tls_conf_t, tls_min_version), .dflt = "1.0" },

{ FR_CONF_OFFSET("certificate", FR_TYPE_SUBSECTION | FR_TYPE_MULTI, fr_tls_conf_t, key_pairs),
.subcs_size = sizeof(fr_tls_conf_key_pair_t), .subcs_type = "fr_tls_conf_key_pair_t",
.subcs = tls_cert_pair_config },

CONF_PARSER_TERMINATOR
};

#ifdef __APPLE__
/*
* We don't want to put the private key password in eap.conf, so check
* for our special string which indicates we should get the password
* programmatically.
*/
static char const *special_string = "Apple:UsecertAdmin";

/** Use cert_admin to retrieve the password for the private key
*
*/
static int conf_cert_admin_password(fr_tls_conf_t *conf)
{
if (!conf->private_key_password) return 0;
size_t i, cnt;

/*
* We don't want to put the private key password in eap.conf, so check
* for our special string which indicates we should get the password
* programmatically.
*/
char const *special_string = "Apple:UsecertAdmin";
if (strncmp(conf->private_key_password, special_string, strlen(special_string)) == 0) {
char cmd[256];
char *password;
long const max_password_len = 128;
FILE *cmd_pipe;
if (!conf->key_pairs) return 0;

cnt = talloc_array_length(conf->key_pairs);
for (i = 0; i < cnt; i++) {
char cmd[256];
char *password;
long const max_password_len = 128;
FILE *cmd_pipe;

if (!conf->key_pairs[i]->password) continue;

if (strncmp(conf->key_pairs[i]->password, special_string, strlen(special_string)) != 0) continue;

snprintf(cmd, sizeof(cmd) - 1, "/usr/sbin/certadmin --get-private-key-passphrase \"%s\"",
conf->private_key_file);
conf->key_pairs[i]->private_key_file);

DEBUG2("Getting private key passphrase using command \"%s\"", cmd);

cmd_pipe = popen(cmd, "r");
if (!cmd_pipe) {
ERROR("%s command failed: Unable to get private_key_password", cmd);
ERROR("Error reading private_key_file %s", conf->private_key_file);
ERROR("Error reading private_key_file %s", conf->key_pairs[i]->private_key_file);
return -1;
}

talloc_const_free(conf->private_key_password);
password = talloc_array(conf, char, max_password_len);
if (!password) {
ERROR("Can't allocate space for private_key_password");
ERROR("Error reading private_key_file %s", conf->private_key_file);
ERROR("Error reading private_key_file %s", conf->key_pairs[i]->private_key_file);
pclose(cmd_pipe);
return -1;
}
Expand All @@ -211,7 +237,8 @@ static int conf_cert_admin_password(fr_tls_conf_t *conf)
password[strlen(password) - 1] = '\0';

DEBUG3("Password from command = \"%s\"", password);
conf->private_key_password = password;
talloc_const_free(conf->key_pairs[i]->password);
conf->key_pairs[i]->password = password;
}

return 0;
Expand Down Expand Up @@ -328,7 +355,7 @@ fr_tls_conf_t *tls_conf_parse_server(CONF_SECTION *cs)
/*
* Initialize TLS
*/
conf->ctx = talloc_array(conf, SSL_CTX *, conf->ctx_count);
conf->ctx = talloc_zero_array(conf, SSL_CTX *, conf->ctx_count);
for (i = 0; i < conf->ctx_count; i++) {
conf->ctx[i] = tls_ctx_alloc(conf, false);
if (conf->ctx[i] == NULL) goto error;
Expand Down

0 comments on commit e8df16d

Please sign in to comment.