diff mbox series

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

Message ID 20180627175844.2809-3-qiuyu.xiao.qyx@gmail.com
State Changes Requested
Headers show
Series IPsec support for tunneling | expand

Commit Message

Qiuyu Xiao June 27, 2018, 5:58 p.m. UTC
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@gmail.com>
---
 Documentation/howto/ipsec.rst |  78 ++++++++++++++++---
 ipsec/ovs-monitor-ipsec       | 138 +++++++++++++++++++++-------------
 2 files changed, 156 insertions(+), 60 deletions(-)

Comments

0-day Robot June 27, 2018, 6:56 p.m. UTC | #1
Bleep bloop.  Greetings Qiuyu Xiao, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 80 characters long (recommended limit is 79)
#60 FILE: Documentation/howto/ipsec.rst:81:
ovs-monitor-ipsec configures strongSwan accordingly based on the tunnel options.

WARNING: Line is 84 characters long (recommended limit is 79)
#217 FILE: ipsec/ovs-monitor-ipsec:230:
        (cert, key, ca_cert) = (keyer.public_cert, keyer.private_key, keyer.ca_cert)

Lines checked: 443, Warnings: 2, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Aaron Conole June 27, 2018, 8:12 p.m. UTC | #2
Qiuyu Xiao <qiuyu.xiao.qyx@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@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.

>  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?

>      unixctl_config_tmpl = Template("""\
> -  Remote IP:      $remote_ip
>    Tunnel Type:    $tunnel_type
> +  Remote IP:      $remote_ip

Why is this change made?

>    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?

>  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.

>  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()
Qiuyu Xiao June 27, 2018, 8:52 p.m. UTC | #3
Hi Aaron,

Thanks for your comments!

On Wed, Jun 27, 2018 at 1:12 PM, Aaron Conole <aconole@redhat.com> wrote:
>
> Qiuyu Xiao <qiuyu.xiao.qyx@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@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()
Ansis July 11, 2018, 1:16 a.m. UTC | #4
On Wed, 27 Jun 2018 at 10:59, Qiuyu Xiao <qiuyu.xiao.qyx@gmail.com> 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.


> Signed-off-by: Qiuyu Xiao <qiuyu.xiao.qyx@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
> +=======================================
>
>  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?

> +                                     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.

> +
> +      % 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?

> +
> +   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.
> +        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?

> -            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
>
Qiuyu Xiao July 11, 2018, 3:44 p.m. UTC | #5
Thanks for your review!

On Tue, Jul 10, 2018 at 6:16 PM, Ansis Atteka <ansisatteka@gmail.com> wrote:
> On Wed, 27 Jun 2018 at 10:59, Qiuyu Xiao <qiuyu.xiao.qyx@gmail.com> 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 <qiuyu.xiao.qyx@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
>> +=======================================
>>
>>  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
>>
diff mbox series

Patch

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 \
+                                     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""")}
 
     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"]:
+
+        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.")
 
 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()