From 1f45a4cfe515c790af2e7377668a89be8bddb9f4 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 9 Jul 2012 22:22:20 -0700 Subject: [PATCH 1/4] enforce some protocol sanity checks --- server-ca/chocolate.py | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/server-ca/chocolate.py b/server-ca/chocolate.py index fc651535f..495280ae8 100755 --- a/server-ca/chocolate.py +++ b/server-ca/chocolate.py @@ -133,12 +133,29 @@ class session(object): def handlesession(self, m, r): if r.failure.IsInitialized(): return - # TODO: perhaps some code belongs here to enforce rules about which - # combinations of protocol messages can occur together. I think the - # rules are: Client must send either nothing (polling for updates) - # or exactly one of request, failure, or completedchallenge. Client - # may not send proceed, challenge, or success. If the rules are - # violated, we should self.die(r, r.BadRequest) and return. + # Note that m.challenge and m.completedchallenge present + # as lists, which are True if they are nonempty. By + # contrast, m.proceed, m.success, m.request, and m.failure + # are always True but have an .IsInitialized() property + # indicating whether they are actually present in m as + # messages from the client. + # + # Check for some ways in which the message from the client + # can be inappropriate. + if m.challenge or m.proceed.IsInitialized() or m.success.IsInitialized(): + self.die(r, r.BadRequest, uri="https://ca.example.com/failures/invalidfromclient") + return + distinct_messages = 0 + if m.request.IsInitialized(): distinct_messages += 1 + if m.failure.IsInitialized(): distinct_messages += 1 + if m.completedchallenge: distinct_messages += 1 + if distinct_messages > 1: + self.die(r, r.BadRequest, uri="https://ca.example.com/failures/mixedmessages") + return + # The rule that a new session must contain a request is enforced + # by handlenewsession. The rule that an existing session must + # not contain a request is enforced by handleexistingsession. + # TODO: check that there are no bad cases that slip through. if m.session == "": # New session r.session = random() @@ -269,8 +286,7 @@ class session(object): def send_challenges(self, m, r): if r.failure.IsInitialized(): return - # TODO: This needs a more sophisticated notion of success/failure, - # and also of the possibility of multiple data strings. + # TODO: This needs a more sophisticated notion of success/failure. for c in self.challenges(): # Currently, we can only handle challenge type 0 (dvsni) # TODO: unify names "succeeded" vs. "satisfied"? From ee9343469f4f0047d7e74e7815ec7995e2548951 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 9 Jul 2012 22:23:44 -0700 Subject: [PATCH 2/4] more explicit clarification about concurrency --- server-ca/daemon.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/server-ca/daemon.py b/server-ca/daemon.py index 692072309..ea6df166b 100644 --- a/server-ca/daemon.py +++ b/server-ca/daemon.py @@ -12,12 +12,12 @@ # The queue mechanism with pending-* is supposed to control # concurrency issues properly, but this needs verification # to ensure that there are no possible race conditions. -# Generally, the server is not supposed to change sessions -# very much once they have been added to a queue, except -# for marking them no longer live if the server realizes -# that something bad has happened to them. There may be -# some exceptions, and they should all be analyzed for -# possible races. +# Generally, the server process (as distinct from the daemon) +# is not supposed to change sessions at all once they have +# been added to a queue, except for marking them no longer +# live if the server realizes that something bad has happened +# to them. There may be some exceptions, and they should all +# be analyzed for possible races. # TODO: The daemon should probably check for timeouts before # advancing sessions' state. Currently timeouts can only From cfc4cbaae347743479f9abfff05964b12dba3620 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Mon, 9 Jul 2012 22:33:02 -0700 Subject: [PATCH 3/4] remove disused debug flag in protocol --- server-ca/chocolate.py | 6 +----- server-ca/chocolate_protocol.proto | 4 ---- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/server-ca/chocolate.py b/server-ca/chocolate.py index 495280ae8..6d14f15a0 100755 --- a/server-ca/chocolate.py +++ b/server-ca/chocolate.py @@ -335,11 +335,7 @@ class session(object): # or similar. # Send reply - if m.debug: - web.header("Content-type", "text/plain") - return "SAW MESSAGE: %s\nRESPONSE: %s\n" % (str(m), str(r)) - else: - return r.SerializeToString() + return r.SerializeToString() def GET(self): web.header("Content-type", "text/html") diff --git a/server-ca/chocolate_protocol.proto b/server-ca/chocolate_protocol.proto index a4d1543f2..2bc75bbd2 100644 --- a/server-ca/chocolate_protocol.proto +++ b/server-ca/chocolate_protocol.proto @@ -106,8 +106,4 @@ message chocolatemessage { /* Sent by SERVER to issue the requested certificate */ optional Success success = 8; - - /* For debugging; should be removed in final protocol. */ - optional bool debug = 9; /* Causes server to return text instead of - message! */ } From 3f03dc4e5c6ff9b59c4c235ebf169705578e17b1 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Tue, 10 Jul 2012 00:25:11 -0700 Subject: [PATCH 4/4] implement dvsni challenge checking --- server-ca/daemon.py | 44 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 7 deletions(-) diff --git a/server-ca/daemon.py b/server-ca/daemon.py index ea6df166b..5fa03c168 100644 --- a/server-ca/daemon.py +++ b/server-ca/daemon.py @@ -102,14 +102,47 @@ def testchallenge(session): # that it has completed the challenges. Information about # the client's reporting could be stored in the database. # Then the CA doesn't need to poll prematurely. - if False: # if challenges all succeed + all_satisfied = True + for i, name in enumerate(r.lrange("%s:names" % session, 0, -1)): + challenge = "%s:%d" % (session, i) + challtime = r.hget(challenge, "challtime") + challtype = r.hget(challenge, "type") + name = r.hget(challenge, "name") + satisfied = r.hget(challenge, "satisfied") == "True" + failed = r.hget(challenge, "failed") == "True" + # TODO: check whether this challenge is too old + if not satisfied and not failed: + if challtype == 0: # DomainValidateSNI + dvsni_nonce = r.hget(challenge, "dvsni:nonce") + dvsni_r = r.hget(challenge, "dvsni:r") + dvsni_ext = r.hget(challenge, "dvsni:ext") + if verify_challenge(name, dvsni_r, dvsni_nonce)[0]: + r.hset(challenge, "satisfied", True) + else: + all_satisfied = False + # TODO: distinguish permanent and temporarily failures + # can cause a permanent failure under some conditions, causing + # the session to become dead. TODO: need to articulate what + # those conditions are + else: + # Don't know how to handle this challenge type + all_satisfied = False + elif not satisfied: + all_satisfied = False + if all_satisfied: + # Challenges all succeeded, so we should prepare to issue + # the requested cert. + # TODO: double-check that there were > 0 challenges, + # so that we don't somehow mistakenly issue a cert in + # response to an empty list of challenges (even though + # the daemon that put this session on the queue should + # also have implicitly guaranteed this). r.hset(session, "state", "issue") r.lpush("pending-issue", session) else: + # Some challenges are not verified. + # Put this session back on the stack to try to verify again. r.lpush("pending-testchallenge", session) - # can also cause a failure under some conditions, causing the - # session to become dead. TODO: need to articulate what those - # conditions are def issue(session): if r.hget(session, "live") != "True": @@ -130,9 +163,6 @@ def issue(session): # should never happen. r.lrem("pending-requests", session) return - # Note that we can push this back into the original queue. - # TODO: need to add a way to make sure we don't test the same - # TODO: actually make this call issue the cert csr = r.hget(session, "csr") cert = CSR.issue(csr) r.hset(session, "cert", cert)