Thanks for your review! On Tue, Jul 10, 2018 at 6:16 PM, Ansis Atteka <[email protected]> wrote: > On Wed, 27 Jun 2018 at 10:59, Qiuyu Xiao <[email protected]> wrote: >> >> This patch adds CA-cert based authentication to the ovs-monitor-ipsec >> daemon. With CA-cert based authentication enabled, OVS approves IPsec >> tunnel if the peer has a cert signed by a trusted CA and the identity of >> the peer cert is as expected. Belows are the major changes and the >> reasons: >> >> 1) Added CA-cert based authentication. Compared with peer-cert based >> authentication, this one doesn't need to import peer cert to the local >> host to do configuration. This is especially beneficial if host has >> mutiple peers and peers frequently update their certs. This feature is >> required for the upcoming OVN IPsec support. >> >> 2) Changed the host cert and private key configuration interface. >> Previously, the host's cert and private key can be configured in either >> Open_vSwitch's SSL column or the SSL table. Now, the host certificate >> and private key can be only configured in the Open_vSwitch table's >> other_config column. Since it is not SSL cert and key, we'd better not >> to confuse users by saying so. >> >> 3) Changed the peer cert configuration interface. Previously, the peer >> cert is configured by setting the interface's options column as the >> content of the peer cert. It's changed to setting the column as the path >> of the peer cert. This is easier to be configured by the command line >> tool, and is consistent with other cert and key configuration interface >> which is better from a usability point of view. >> > > Would you mind creating a patch ovs/poc/ipsec ansible+vagrant recipe > that deploys two VMs, installs strongswan, openvswitch and then > configure IPsec between them? > Current tests use mocked strongSwan's ipsec utility.
This sounds a more solid test compared to the simulated one. I will try to create such test. > > >> Signed-off-by: Qiuyu Xiao <[email protected]> >> --- >> Documentation/howto/ipsec.rst | 78 ++++++++++++++++--- >> ipsec/ovs-monitor-ipsec | 138 +++++++++++++++++++++------------- >> 2 files changed, 156 insertions(+), 60 deletions(-) >> >> diff --git a/Documentation/howto/ipsec.rst b/Documentation/howto/ipsec.rst >> index 4e4f4d211..b42312da5 100644 >> --- a/Documentation/howto/ipsec.rst >> +++ b/Documentation/howto/ipsec.rst >> @@ -21,9 +21,9 @@ >> >> Avoid deeper levels because they do not render well. >> >> -============================================== >> -How to Encrypt Open vSwitch Tunnels with IPsec >> -============================================== >> +======================================= >> +Encrypt Open vSwitch Tunnels with IPsec >> +======================================= >> >> This document describes how to use Open vSwitch integration with strongSwan >> 5.1 or later to provide IPsec security for STT, GENEVE, GRE and VXLAN >> tunnels. >> @@ -77,6 +77,67 @@ Install strongSwan and openvswitch-ipsec debian packages:: >> Configuration >> ------------- >> >> +The IPsec configuration is done by setting options of the tunnel interface. >> +ovs-monitor-ipsec configures strongSwan accordingly based on the tunnel >> options. >> + >> +Authentication Methods >> +~~~~~~~~~~~~~~~~~~~~~~ >> + >> +Hosts of the IPsec tunnel need to authenticate each other to build a secure >> +channel. There are three authentication methods: >> + >> +1) You can set a pre-shared key in both hosts to do authentication. This >> + method is easier to use but less secure:: >> + >> + % ovs-vsctl add-port br0 ipsec_gre0 -- \ >> + set interface ipsec_gre0 type=gre \ >> + options:remote_ip=1.2.3.4 \ >> + options:psk=swordfish]) >> + >> +2) You can use peer certificates to do authentication. First, generate >> + certificate and private key in each host. The certificate could be >> + self-signed. Refer to the ovs-pki(8) man page for more information >> + regarding certificate and key generation. Then, copy the peer certificate >> + to the local host and type:: >> + >> + % ovs-vsctl set Open_vSwitch . \ >> + other_config:certificate=/path/to/local_cert.pem \ >> + other_config:private_key=/path/to/priv_key.pem >> + % ovs-vsctl add-port br0 ipsec_gre0 -- \ >> + set interface ipsec_gre0 type=gre \ >> + options:remote_ip=1.2.3.4 \ >> + options:pki=peer_auth \ > It seems you have introduced options:pki to allow to pick between > self-signed certificate configuration and certificate authority signed > certificate configuration. Instead, could you determine the > configuration from some fields that you uniquely set in each > configuration? Yes. I think the pki option is not needed. I will change this. >> + >> options:peer_cert=/path/to/peer_cert.pem >> + >> + `local_cert.pem` is the certificate of the local host. `priv_key.pem` >> + is the private key of the local host. `priv_key.pem` needs to be stored >> in >> + a secure location. `peer_cert.pem` is the certificate of the remote host. >> + >> +3) You can also use CA certificate to do authentication. First, you need to >> + establish your public key infrastructure. The certificate of each host >> + needs to be signed by the CA certificate. Refer to the ovs-pki(8) man >> page > s/CA certificate/Certificate Authority > >> + for more information regarding PKI establishment. Then, copy the CA >> + certificate to the local host and type:: > > You don't seem to specify where on filesystem CA certificate must be > copied so that strongSwan picks it up. strongSwan can find the certificate if it's specified with absolute path. > >> + >> + % ovs-vsctl set Open_vSwitch . \ >> + other_config:certificate=/path/to/local_cert.pem \ >> + other_config:private_key=/path/to/priv_key.pem \ >> + other_config:ca_cert=/path/to/ca_cert.pem >> + % ovs-vsctl add-port br0 ipsec_gre0 -- \ >> + set interface ipsec_gre0 type=gre \ >> + options:remote_ip=1.2.3.4 \ >> + options:pki=ca_auth \ >> + options:local_name=local_cn \ >> + options:remote_name=remote_cn > Is local_cn redundant (ie implied rom Open_vSwitch:other_config:certificate)? > > Are the local_cn and remote_cn really neccessary? IIRC strongswan > worked fine without that. Have things changed recently? We should be able to extract local_cn from the configured certificate. I didn't write the code to do so. I will try to change this. local_cn and remote_cn are necessary for the CA-based authentication. We need to write remote_cn to the configuration file to make sure only the certificate with this common name can be authenticated, instead of all other certificates with valid CA signature. >> + >> + strongSwan extracts identity from the certificate's `subjectAltName` >> field, >> + so you have to use x509 v3 certificate. `local_cn` is the hostname from >> the >> + `subjectAltName` field of the local host certificate. `remote_cn` is the >> + hostname from the `subjectAltName` field of the peer host certificate. >> + >> +Forwarding Modes >> +~~~~~~~~~~~~~~~~ >> + >> IPsec with ovs-monitor-ipsec daemon can be configured in three >> different forwarding policy modes: >> >> @@ -116,7 +177,7 @@ different forwarding policy modes: >> ovs-monitor-ipsec installed IPsec policies. >> However, assumption here is that OpenFlow controller was careful >> and installed OpenFlow rule with set SKB mark action specified in >> - OVSDB Open_vSwitch table before the first packets were able to leave >> + OVSDB Open_vSwitch table before the first packet was able to leave >> the OVS tunnel. >> >> 3) ovs-monitor-ipsec assumes that packets coming from all OVS tunnels >> @@ -126,7 +187,7 @@ different forwarding policy modes: >> >> % ovs-vsctl set Open_vSwitch . other_config:default_ipsec_skb_mark=0/1 >> % ovs-vsctl add-port br0 ipsec_gre0 -- \ >> - set interface gre0 type=gre \ >> + set interface ipsec_gre0 type=gre \ >> options:remote_ip=1.2.3.4 \ >> options:psk=swordfish]) >> >> @@ -189,11 +250,10 @@ If you think you may have found a bug with security >> implications, like >> 1) IPsec protected tunnel accepted packets that came unencrypted; OR >> 2) IPsec protected tunnel allowed packets to leave unencrypted; >> >> -Then report such bugs according to `security guidelines >> -<Documentation/internals/security.rst>`__. >> +Then report such bugs according to :doc:`/internals/security`. >> >> If bug does not have security implications, then report it according to >> -instructions in `<REPORTING-bugs.rst>`__. >> +instructions in :doc:`/internals/bugs`. >> >> There is also a possibility that there is a bug in strongSwan. In that >> -case report it to strongSwan mailing list. >> \ No newline at end of file >> +case report it to strongSwan mailing list. >> diff --git a/ipsec/ovs-monitor-ipsec b/ipsec/ovs-monitor-ipsec >> index 322a7045a..65f360a80 100755 >> --- a/ipsec/ovs-monitor-ipsec >> +++ b/ipsec/ovs-monitor-ipsec >> @@ -179,21 +179,30 @@ $auth_section >> left=$local_ip >> right=$remote_ip >> authby=psk"""), >> - "rsa" : Template("""\ >> + "pki_peer" : Template("""\ >> left=$local_ip >> right=$remote_ip >> - rightcert=ovs-$remote_ip.pem >> - leftcert=$certificate""")} >> + leftcert=$certificate >> + rightcert=$peer_cert"""), >> + "pki_ca" : Template("""\ >> + left=$local_ip >> + right=$remote_ip >> + leftcert=$certificate >> + leftid=$local_name >> + rightid=$remote_name""")} >> >> unixctl_config_tmpl = Template("""\ >> - Remote IP: $remote_ip >> Tunnel Type: $tunnel_type >> + Remote IP: $remote_ip >> Local IP: $local_ip >> SKB mark: $skb_mark >> - Use SSL cert: $use_ssl_cert >> - My cert: $certificate >> - My key: $private_key >> - His cert: $peer_cert >> + Local cert: $certificate >> + Local key: $private_key >> + CA cert: $ca_cert >> + Remote cert: $peer_cert >> + Remote name: $remote_name >> + Local name: $local_name >> + PKI: $pki >> PSK: $psk >> """) >> >> @@ -209,7 +218,6 @@ $auth_section >> self.state = "INIT" >> self.conf = {} >> self.status = {} >> - self.cert_file = None >> self.update_conf(row) >> >> def update_conf(self, row): >> @@ -219,11 +227,7 @@ $auth_section >> False.""" >> ret = False >> options = row.options >> - use_ssl_cert = options.get("use_ssl_cert") == "true" >> - cert = options.get("certificate") >> - key = options.get("private_key") >> - if use_ssl_cert: # Override with SSL certs if told so >> - (cert, key) = (keyer.public_cert, keyer.private_key) >> + (cert, key, ca_cert) = (keyer.public_cert, keyer.private_key, >> keyer.ca_cert) >> >> new_conf = { >> "ifname" : self.name, >> @@ -233,8 +237,11 @@ $auth_section >> "skb_mark" : keyer.wanted_skb_mark, >> "certificate" : cert, >> "private_key" : key, >> - "use_ssl_cert" : use_ssl_cert, >> + "ca_cert" : ca_cert, >> "peer_cert" : options.get("peer_cert"), >> + "remote_name" : options.get("remote_name"), >> + "local_name" : options.get("local_name"), >> + "pki" : options.get("pki"), >> "psk" : options.get("psk")} >> if self.conf != new_conf: >> # Configuration was updated in OVSDB. Validate it and figure >> @@ -303,7 +310,10 @@ $auth_section >> else: >> secrets.write("%s : RSA %s\n" % (self.conf["remote_ip"], >> self.conf["private_key"])) >> - auth_section = self.auth_tmpl["rsa"].substitute(self.conf) >> + if self.conf["pki"] == "ca_auth": >> + auth_section = >> self.auth_tmpl["pki_ca"].substitute(self.conf) >> + else: >> + auth_section = >> self.auth_tmpl["pki_peer"].substitute(self.conf) >> vals = self.conf.copy() >> vals["auth_section"] = auth_section >> vals["version"] = self.version >> @@ -344,45 +354,54 @@ $auth_section >> pass malformed configuration to strongSwan.""" >> >> self.invalid_reason = None >> + >> if not self.conf["remote_ip"]: >> self.invalid_reason = ": 'remote_ip' is not set" >> - elif self.conf["peer_cert"]: >> + > I would use elif instead of if here. Otherwise, you do the invalid > configuration test multiple times and store only the last one. Good point. I will change this. >> + if self.conf["pki"]: >> if self.conf["psk"]: >> self.invalid_reason = ": 'psk' must be unset with PKI" >> elif not self.conf["certificate"]: >> self.invalid_reason = ": must set 'certificate' with PKI" >> elif not self.conf["private_key"]: >> self.invalid_reason = ": must set 'private_key' with PKI" >> + >> + if self.conf["pki"] == "ca_auth": >> + if not self.conf["ca_cert"]: >> + self.invalid_reason = ": must set 'ca_cert' "\ >> + "for ca_cert-based authentication" >> + elif not self.conf["local_name"]: >> + self.invalid_reason = ": must set 'local_name' "\ >> + "for ca_cert-based authentication" >> + elif not self.conf["remote_name"]: >> + self.invalid_reason = ": must set 'remote_name' "\ >> + "for ca_cert-based authentication" >> + elif self.conf["pki"] == "peer_auth": >> + if not self.conf["peer_cert"]: >> + self.invalid_reason = ": must set 'peer_cert' "\ >> + "for peer_cert-based authentication" >> + else: >> + self.invalid_reason = ": must set 'pki' "\ >> + "as 'ca_auth' or 'peer_auth'." >> elif self.conf["psk"]: >> - if self.conf["certificate"] or self.conf["private_key"]: >> - self.invalid_reason = ": 'certificate', 'private_key' and "\ >> - "'use_ssl_cert' must be unset with >> PSK" >> + if self.conf["certificate"] or self.conf["private_key"] \ >> + or self.conf["ca_cert"] or self.conf["peer_cert"]: >> + self.invalid_reason = ": 'certificate', 'private_key', "\ >> + "'ca_cert' and 'peer_cert' must be unset with PSK" >> + else: >> + self.invalid_reason = ": must use 'pki' or 'psk' option "\ >> + "for authentication" >> + >> if self.invalid_reason: >> return False >> + >> return True >> >> def _cleanup_old_state(self): >> - if self.cert_file: >> - try: >> - os.remove(self.cert_file) > I am confused why you removed this line. Are you removing the old > certificate form filesystem somewhere else or is this not supported > configuration anymore where ovs-monitor-ipsec copies peer certificate > content onto filesystem for strongSwan to consume? I changed the configuration by letting user specify the path of the cert in ovsdb instead of storing the content of the cert in ovsdb. I think it's easier to do such configuration in command line. > >> - except OSError: >> - vlog.warn("could not remove '%s'" % (self.cert_file)) >> - self.cert_file = None >> + vlog.info("Clean old state.") > I don't think you should log this at info level. > >> >> def _push_new_state(self): >> - if self.conf["peer_cert"]: >> - fn = keyer.CERT_DIR + "/ovs-%s.pem" % (self.name) >> - try: >> - cert = open(fn, "w") >> - try: >> - cert.write(self.conf["peer_cert"]) >> - finally: >> - cert.close() >> - vlog.info("created peer certificate %s" % (fn)) >> - self.cert_file = fn >> - except (OSError, IOError) as e: >> - vlog.err("could not create certificate '%s'" % (fn)) >> - >> + vlog.info("Push new state.") > I don't think you should log this at info level. > >> >> class StrongSwanKeyer(object): >> """This class is a wrapper around strongSwan.""" >> @@ -407,18 +426,33 @@ conn %%default >> SHUNT_POLICY = """conn prevent_unencrypted_gre >> type=drop >> leftprotoport=gre >> - mark=%s >> + mark={0} >> + >> +conn prevent_unencrypted_geneve >> + type=drop >> + leftprotoport=udp/6081 >> + mark={0} >> >> conn prevent_unencrypted_stt >> type=drop >> leftprotoport=tcp/7471 >> - mark=%s >> + mark={0} >> + >> +conn prevent_unencrypted_vxlan >> + type=drop >> + leftprotoport=udp/4789 >> + mark={0} >> + >> +""" >> + CA_SECTION = """ca ca_auth >> + cacert=%s >> >> """ >> >> def __init__(self, strongswan_root_prefix): >> self.private_key = None >> self.public_cert = None >> + self.ca_cert = None >> self.wanted_skb_mark = None >> self.in_use_skb_mark = None >> self.tunnels = {} >> @@ -436,7 +470,7 @@ conn prevent_unencrypted_stt >> def is_ipsec_required(self, options_column): >> """Return True if tunnel needs to be encrypted. Otherwise, >> returns False.""" >> - return "psk" in options_column or "peer_cert" in options_column >> + return "psk" in options_column or "pki" in options_column >> >> def restart(self): >> """This function restarts strongSwan.""" >> @@ -465,6 +499,7 @@ conn prevent_unencrypted_stt >> """Updates already existing """ >> self.public_cert = credentials[0] >> self.private_key = credentials[1] >> + self.ca_cert = credentials[2] >> >> def update_skb_mark(self, skb_mark): >> """Update skb_mark used in IPsec policies.""" >> @@ -499,8 +534,12 @@ conn prevent_unencrypted_stt >> needs_refresh = True >> >> if self.in_use_skb_mark: >> - ipsec_conf.write(" mark_out=%s\n" % self.in_use_skb_mark) >> - ipsec_conf.write(self.SHUNT_POLICY % (self.in_use_skb_mark, >> self.in_use_skb_mark)) >> + ipsec_conf.write(self.SHUNT_POLICY.format(self.in_use_skb_mark)) >> + >> + # No need to set needs_refresh here if ca_cert is changed. The >> tunnel >> + # iteration will set needs_refresh. >> + if self.ca_cert: >> + ipsec_conf.write(self.CA_SECTION % self.ca_cert) >> >> for name, tunnel in self.tunnels.iteritems(): >> if tunnel.run(): >> @@ -565,7 +604,6 @@ conn prevent_unencrypted_stt >> vlog.info("%s is outdated %u" % (conn, ver)) >> subprocess.call([self.IPSEC, "stroke", "down-nb", conn]) >> >> - >> def _get_strongswan_conns(self): >> """This function parses output from 'ipsec status' command. >> It returns dictionary where <key> is interface name (as in OVSDB) >> @@ -612,11 +650,12 @@ conn prevent_unencrypted_stt >> def read_ovsdb_open_vswitch_table(data): >> """This functions reads IPsec relevant configuration from Open_vSwitch >> table.""" >> - ssl = (None, None) >> + ssl = (None, None, None) >> global_ipsec_skb_mark = None >> for row in data["Open_vSwitch"].rows.itervalues(): >> - if row.ssl: >> - ssl = (row.ssl[0].certificate, row.ssl[0].private_key) >> + ssl = (row.other_config.get("certificate"), \ >> + row.other_config.get("private_key"), \ >> + row.other_config.get("ca_cert")) >> global_ipsec_skb_mark = >> row.other_config.get("default_ipsec_skb_mark") >> keyer.update_ssl_credentials(ssl) >> keyer.update_skb_mark(global_ipsec_skb_mark) >> @@ -646,7 +685,6 @@ def read_ovsdb(data): >> read_ovsdb_open_vswitch_table(data) >> read_ovsdb_interface_table(data) >> >> - >> def main(): >> parser = argparse.ArgumentParser() >> parser.add_argument("database", metavar="DATABASE", >> @@ -674,7 +712,6 @@ def main(): >> ["name", "type", "options", "cfm_fault", >> "ofport"]) >> schema_helper.register_columns("Open_vSwitch", ["ssl", "other_config"]) >> - schema_helper.register_columns("SSL", ["certificate", "private_key"]) >> idl = ovs.db.idl.Idl(remote, schema_helper) >> >> ovs.daemon.daemonize() >> @@ -715,7 +752,6 @@ def main(): >> unixctl_server.close() >> idl.close() >> >> - >> if __name__ == '__main__': >> try: >> main() >> -- >> 2.17.1 >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
