Thanks for your review!

On Tue, Jul 10, 2018 at 6:16 PM, Ansis Atteka <ansisatt...@gmail.com> wrote:
> On Wed, 27 Jun 2018 at 10:59, Qiuyu Xiao <qiuyu.xiao....@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....@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
>>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to