Hi Aaron, Thanks for your comments!
On Wed, Jun 27, 2018 at 1:12 PM, Aaron Conole <[email protected]> wrote: > > Qiuyu Xiao <[email protected]> writes: > > > 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. > > > > 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 > > +======================================= > > It seems odd to introduce something and then cut it in the very next > patch. Please don't do this. Most of the diffs in this file can be > folded into patch 1/3 - please do that instead of fixing things later in > the series. Patch 1/3 is from Ansis. I separated the patches so he will know the changes I made. But it makes sense to make 1/3 and 2/3 a single patch. I will change that in the next version. > > > > 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 \ > > + > > 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 > > + for more information regarding PKI establishment. Then, copy the CA > > + 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 \ > > + 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 > > + > > + 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""")} > > This diff block is difficult to follow. There is a re-order of the > options mixed with a change in values. Can you separate those changes? Will do it. > > unixctl_config_tmpl = Template("""\ > > - Remote IP: $remote_ip > > Tunnel Type: $tunnel_type > > + Remote IP: $remote_ip > > Why is this change made? umm, there is not much need to change this. > > > 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"]: > > + > > + 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) > > - except OSError: > > - vlog.warn("could not remove '%s'" % (self.cert_file)) > > - self.cert_file = None > > + vlog.info("Clean old state.") > > > > 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.") > > These two functions don't seem useful anymore. Wouldn't it be better to > remove them? > Yes, I will remove it. > > 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) > > > > - > > This will cause a flake8 violation. > I will fix this. > > 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() > > > > - > > This will cause a flake8 violation. > > > if __name__ == '__main__': > > try: > > main() _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
