Implement discussed changes in #735. #737

certxlm · 2019-10-03T08:30:03Z

Added 3 new Client context configuration:

transport_ca_certificate: a list of PEM certificates to valide the
frontend certificate (transport)
transport_private_key: a private key for client cert authentication
transport_public_certificate: the certificate for client cert
authentication

Added 3 new Client context configuration: - transport_ca_certificate: a list of PEM certificates to valide the frontend certificate (transport) - transport_private_key: a private key for client cert authentication - transport_public_certificate: the certificate for client cert authentication

googlebot · 2019-10-03T08:30:08Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

certxlm · 2019-10-03T08:52:00Z

@googlebot I signed it!

googlebot · 2019-10-03T08:52:27Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

mathieubaeumler · 2019-10-03T08:54:57Z

@googlebot I consent.

googlebot · 2019-10-03T08:55:00Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

mbushkov

Thank you for this PR. The code generally looks good, but I think we can make it a bit simpler and cleaner. Please see my comments below.

Also, an important issue: this functionality has to be tested in order to be submitted. We have an integration test for AdminUI that instantiates an HTTPS server (

grr/grr/server/grr_response_server/gui/api_integration_tests/ssl_test.py

Line 37 in 6dff4bb

class ApiSslServerTestBase(test_lib.GRRBaseTest, acl_test_lib.AclTestMixin):

). I wonder if we can use a similar approach to test comms changes introduced here?

At the very least, we have to augment

grr/grr/client/grr_response_client/comms_test.py

Line 98 in d399ef6

class HTTPManagerTest(test_lib.GRRBaseTest):

to make sure expected parameters are passed to "requests" library when OpenURL is called with newly introduced arguments

mbushkov · 2019-10-07T13:16:30Z

grr/client/grr_response_client/comms.py

+                         transport_cert=None,
+                         transport_key=None,
+                         transport_cacert=None,


There's a bunch of places where you have to pass transport_cert, transport_key and transport_cacert. Would it make sense to group them into a class? You can use namedtuple for that (like it's done here:

grr/grr/core/grr_response_core/lib/parsers/linux_release_parser.py

Line 24 in b03d9ae

ParsedRelease = collections.namedtuple('ParsedRelease', 'release, major, minor')

)

Maybe makes sense to name the data structure TransportSecurity?

indeed, already working on that point, should be able to provide it soon

mbushkov · 2019-10-07T13:17:06Z

grr/client/grr_response_client/comms.py

+      transport_cert: The certificate to encrypt communications at the transport level
+      transport_key: The private key to encrypt communications at the transport level
+      transport_cacert: The path to the trusted ca certificates files, default to certifi.where()


Please see my comment above. I think it'd make sense to group these 3 arguments into a single one.

mbushkov · 2019-10-07T13:17:21Z

grr/client/grr_response_client/comms.py

+          transport_cert=transport_cert,
+          transport_key=transport_key,
+          transport_cacert=transport_cacert,


Please see the comment above.

mbushkov · 2019-10-07T13:17:27Z

grr/client/grr_response_client/comms.py

+              transport_cert=None,
+              transport_key=None,
+              transport_cacert=None,


Please see the comment above.

mbushkov · 2019-10-07T13:30:27Z

grr/core/grr_response_core/lib/config_lib.py

+  def DEFINE_semantic_value_list(self, semantic_type, name, default=None, help=""):
+    if issubclass(semantic_type, rdf_structs.RDFStruct):
+      raise ValueError("DEFINE_semantic_value_list should be used for types "
+                       "based on RDFValues.")
+    self.AddOption(
+        type_info.RDFValueList(
+            rdfclass=semantic_type,
+            name=name,
+            default=default,
+            validator=type_info.RDFValueType(rdfclass=semantic_type),
+            description=help))
+


IIUC, there's a canonical text representation for certs chains as text. So instead of specifying a list of certificates in GRR configuration, you can likely put the whole chain in a canonical format as a string. This way you won't have to introduce the notion of semantic_value_list. Please see my comment below regarding RDFX509CertChain.

That, on the other hand, I'm really not sure how to tackle.
The only way I know of to represent a cert chain is to actually simply concatenate all the involved certs (think /etc/ssl/certs/ca-certificates.crt).

Which basically makes it a list of x509 certificates, hence that semantic_value_list (which, admittedly, is an horrendous hack which I'm not proud of).

So, could you provide me with some pointers on what exactly is this canonical text representation you're mentioning? Then I'll be glad to implement the requested change and get rid of the list :)

Sorry for a delay with the reply. By canonical text representation I indeed mean a simple concatenation of all the certs.

In your current PR you expect a list of certificates in the configuration and then you concatenate them and pass to the communication layer. But GRR doesn't need to access each individual certificate, it will concatenate them anyway and delegate everything to the "requests" library. With a type like a suggested RDFX509CertChain, you can provide the certificates chain as a concatenated string right in the config and then simply store it in a file and and pass further.

To be fair, I'd also see no problem with simply using a string type and DEFINE_string for Client.transport_ca_certificate. You can then simply read the chain from the config option, put it into a temp file and pass it on.

What do you think?

mbushkov · 2019-10-07T13:40:11Z

grr/client/grr_response_client/comms.py

+    self.transport_cacerts = None
+    try:
+      if transport_cacert is None:
+        logging.debug("loading Client.transport_ca_certificate from configuration file ...")
+        transport_cacert = config.CONFIG.Get("Client.transport_ca_certificate", default=None)
+      if transport_cacert is not None:
+        pem_path = os.path.join(config.CONFIG["Client.install_path"], "certs", "cacert.pem")
+        logging.debug("dumping Client.transport_ca_certificate to %s ..." % pem_path)
+        with open(pem_path, "wb") as fd:
+          for cert in transport_cacert:
+            fd.write(cert.AsPEM().encode("utf-8"))
+            fd.write(os.linesep)
+        self.transport_cacert = pem_path
+      else:
+        logging.debug("Client.transport_ca_certificate is not set, defaulting to bundled CA")
+    except Exception as e:
+      logging.error("unable to save transport cacert: %s", e)


This code and the code below feels repeatable. Can we refactor it into a function?

mbushkov · 2019-10-07T13:47:48Z

grr/client/grr_response_client/comms.py

+        with open(pem_path, "wb") as fd:
+          for cert in transport_cacert:
+            fd.write(cert.AsPEM().encode("utf-8"))
+            fd.write(os.linesep)


Which permissions is the file going to be created with?
I think it's cleaner to use tempfile.NamedTemporaryFile. This way you also won't have to care about the "certs" subfolder in Client.install_path existing or not and there will be no need to modify the client builder code.

That is a nice improvement also, and with delete=true, might even ensure the files are cleaned when grr is stopped. I'll work on that too.

mbushkov · 2019-10-07T13:56:36Z

grr/client/grr_response_client/comms.py

+    transport_cert=self.transport_cert
+    transport_key=self.transport_key
+    transport_cacert=self.transport_cacert
    response = self.http_manager.OpenServerEndpoint(
-        "server.pem", verify_cb=self.VerifyServerPEM)
+        "server.pem",
+        verify_cb=self.VerifyServerPEM,
+        transport_cert=transport_cert,
+        transport_cacert=transport_cacert,
+        transport_key=transport_key)


Why can't we pass self.transport_cert, self.transport_key, self.transport_cacert directly? Why do we need to assign them to variables first?

mbushkov · 2019-10-07T13:56:43Z

grr/client/grr_response_client/comms.py

+    transport_cert=self.transport_cert
+    transport_key=self.transport_key
+    transport_cacert=self.transport_cacert
    # Verify the response is as it should be from the control endpoint.
    response = self.http_manager.OpenServerEndpoint(
        path="control?api=%s" % config.CONFIG["Network.api"],
+        transport_cert=transport_cert,
+        transport_key=transport_key,
+        transport_cacert=transport_cacert,


Why can't we pass self.transport_cert, self.transport_key, self.transport_cacert directly? Why do we need to assign them to variables first?

mbushkov · 2019-10-07T15:39:18Z

grr/core/grr_response_core/lib/type_info.py

+class RDFValueList(List):
+  """A List of RDFValue."""
+  class _validator:
+    def __init__(self, validator=None):
+      self.validator = validator
+    def Validate(self, val):
+      try:
+        return self.validator.Validate(val.encode('utf-8'))
+      except Exception as e:
+        raise TypeValueError("Invalid data: %s" % e)
+  def __init__(self, rdfclass=None, validator=None, **kwargs):
+      super(RDFValueList, self).__init__(**kwargs)
+      self.validator = self._validator(validator)
+      self._type = self.rdfclass = rdfclass
+


This will make an already complicate GRR type info system even more complicated :)
What I'd suggest instead is to implement an RDFX509CertChain class that would know how to parse from/serialize into a canonical representation of a cert chain. The call can be done similarly to the RDFX509Cert (

grr/grr/core/grr_response_core/lib/rdfvalues/crypto.py

Line 67 in c2a442a

class RDFX509Cert(rdfvalue.RDFPrimitive):

). But instead of having cert-specific methods, it will have a "certs" property that will return a list of certs in the chain (each having RDFX509Cert type).
Also, this class would have to be tested to ensure parsing works as expected.

mathieubaeumler · 2019-10-08T07:17:14Z

One last note, I'll try to find some time to implement the tests in comms_test.py and if possible ssl_test.py

mbushkov reviewed Oct 7, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement discussed changes in #735. #737

Implement discussed changes in #735. #737

certxlm commented Oct 3, 2019

googlebot commented Oct 3, 2019

certxlm commented Oct 3, 2019

googlebot commented Oct 3, 2019

mathieubaeumler commented Oct 3, 2019

googlebot commented Oct 3, 2019

mbushkov left a comment

mbushkov Oct 7, 2019

mathieubaeumler Oct 8, 2019

mbushkov Oct 7, 2019

mbushkov Oct 7, 2019

mbushkov Oct 7, 2019

mbushkov Oct 7, 2019

mathieubaeumler Oct 8, 2019

mbushkov Oct 11, 2019

mbushkov Oct 7, 2019

mbushkov Oct 7, 2019

mathieubaeumler Oct 8, 2019

mbushkov Oct 7, 2019

mbushkov Oct 7, 2019

mbushkov Oct 7, 2019

mathieubaeumler commented Oct 8, 2019

Implement discussed changes in #735. #737

Are you sure you want to change the base?

Implement discussed changes in #735. #737

Conversation

certxlm commented Oct 3, 2019

googlebot commented Oct 3, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

certxlm commented Oct 3, 2019

googlebot commented Oct 3, 2019

mathieubaeumler commented Oct 3, 2019

googlebot commented Oct 3, 2019

mbushkov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathieubaeumler commented Oct 8, 2019