[ovs-dev] [PATCH 2/3] ipsec: add CA-cert based authentication

Qiuyu Xiao qiuyu.xiao.qyx at gmail.com
Wed Jun 27 20:52:04 UTC 2018


Hi Aaron,

Thanks for your comments!

On Wed, Jun 27, 2018 at 1:12 PM, Aaron Conole <aconole at redhat.com> wrote:
>
> Qiuyu Xiao <qiuyu.xiao.qyx at gmail.com> 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 <qiuyu.xiao.qyx at gmail.com>
> > ---
> >  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()


More information about the dev mailing list