refactor ocsp request parsing and generation to support only one cert (#4439)

* refactor ocsp request parsing and generation to support only one cert

* small doc change

* notimplementederror

5 files changed, 78 insertions, 177 deletions

diff --git a/docs/x509/ocsp.rst b/docs/x509/ocsp.rst
index afbb2ef75..80abf1660 100644
--- a/docs/x509/ocsp.rst
+++ b/docs/x509/ocsp.rst
@@ -97,8 +97,7 @@ Loading Requests
 
         >>> from cryptography.x509 import ocsp
         >>> ocsp_req = ocsp.load_der_ocsp_request(der_ocsp_req)
-        >>> for request in ocsp_req:
-        ...     print(request.serial_number)
+        >>> print(ocsp_req.serial_number)
         872625873161273451176241581705670534707360122361
 
 
@@ -113,10 +112,10 @@ Creating Requests
     objects.
 
 
-    .. method:: add_request(cert, issuer, algorithm)
+    .. method:: add_certificate(cert, issuer, algorithm)
 
         Adds a request using a certificate, issuer certificate, and hash
-        algorithm.
+        algorithm. This can only be called once.
 
         :param cert: The :class:`~cryptography.x509.Certificate` whose validity
             is being checked.
@@ -141,15 +140,16 @@ Creating Requests
 
         >>> from cryptography.hazmat.backends import default_backend
         >>> from cryptography.hazmat.primitives import serialization
-        >>> from cryptography.hazmat.primitives.hashes import SHA256
+        >>> from cryptography.hazmat.primitives.hashes import SHA1
         >>> from cryptography.x509 import load_pem_x509_certificate, ocsp
         >>> cert = load_pem_x509_certificate(pem_cert, default_backend())
         >>> issuer = load_pem_x509_certificate(pem_issuer, default_backend())
         >>> builder = ocsp.OCSPRequestBuilder()
-        >>> builder = builder.add_request(cert, issuer, SHA256())
+        >>> # SHA1 is in this example because RFC 5019 mandates its use.
+        >>> builder = builder.add_certificate(cert, issuer, SHA1())
         >>> req = builder.build()
         >>> base64.b64encode(req.public_bytes(serialization.Encoding.DER))
-        b'MF8wXTBbMFkwVzANBglghkgBZQMEAgEFAAQgn3BowBaoh77h17ULfkX6781dUDPD82Taj8wO1jZWhZoEINxPgjoQth3w7q4AouKKerMxIMIuUG4EuWU2pZfwih52AgI/IA=='
+        b'MEMwQTA/MD0wOzAJBgUrDgMCGgUABBRAC0Z68eay0wmDug1gfn5ZN0gkxAQUw5zz/NNGCDS7zkZ/oHxb8+IIy1kCAj8g'
 
 
 Interfaces
@@ -159,24 +159,8 @@ Interfaces
 
     .. versionadded:: 2.4
 
-    An ``OCSPRequest`` is an iterable containing one or more
-    :class:`~cryptography.x509.ocsp.Request` objects.
-
-    .. method:: public_bytes(encoding)
-
-        :param encoding: The encoding to use. Only
-            :attr:`~cryptography.hazmat.primitives.serialization.Encoding.DER`
-            is supported.
-
-        :return bytes: The serialized OCSP request.
-
-.. class:: Request
-
-    .. versionadded:: 2.4
-
-    A ``Request`` contains several attributes that create a unique identifier
-    for a certificate whose status is being checked. It may also contain
-    additional extensions (currently unsupported).
+    An ``OCSPRequest`` is an object containing information about a certificate
+    whose status is being checked.
 
     .. attribute:: issuer_key_hash
 
@@ -205,3 +189,11 @@ Interfaces
         :type: int
 
         The serial number of the certificate to check.
+
+    .. method:: public_bytes(encoding)
+
+        :param encoding: The encoding to use. Only
+            :attr:`~cryptography.hazmat.primitives.serialization.Encoding.DER`
+            is supported.
+
+        :return bytes: The serialized OCSP request.
diff --git a/src/cryptography/hazmat/backends/openssl/backend.py b/src/cryptography/hazmat/backends/openssl/backend.py
index cfd7c89fe..64d26afda 100644
--- a/src/cryptography/hazmat/backends/openssl/backend.py
+++ b/src/cryptography/hazmat/backends/openssl/backend.py
@@ -1439,16 +1439,16 @@ class Backend(object):
         ocsp_req = self._lib.OCSP_REQUEST_new()
         self.openssl_assert(ocsp_req != self._ffi.NULL)
         ocsp_req = self._ffi.gc(ocsp_req, self._lib.OCSP_REQUEST_free)
-        for cert, issuer, algorithm in builder._requests:
-            evp_md = self._lib.EVP_get_digestbyname(
-                algorithm.name.encode("ascii"))
-            self.openssl_assert(evp_md != self._ffi.NULL)
-            certid = self._lib.OCSP_cert_to_id(
-                evp_md, cert._x509, issuer._x509
-            )
-            self.openssl_assert(certid != self._ffi.NULL)
-            onereq = self._lib.OCSP_request_add0_id(ocsp_req, certid)
-            self.openssl_assert(onereq != self._ffi.NULL)
+        cert, issuer, algorithm = builder._request
+        evp_md = self._lib.EVP_get_digestbyname(
+            algorithm.name.encode("ascii"))
+        self.openssl_assert(evp_md != self._ffi.NULL)
+        certid = self._lib.OCSP_cert_to_id(
+            evp_md, cert._x509, issuer._x509
+        )
+        self.openssl_assert(certid != self._ffi.NULL)
+        onereq = self._lib.OCSP_request_add0_id(ocsp_req, certid)
+        self.openssl_assert(onereq != self._ffi.NULL)
         return _OCSPRequest(self, ocsp_req)
 
     def elliptic_curve_exchange_algorithm_supported(self, algorithm, curve):
diff --git a/src/cryptography/hazmat/backends/openssl/ocsp.py b/src/cryptography/hazmat/backends/openssl/ocsp.py
index 38e871ece..dd66e36d9 100644
--- a/src/cryptography/hazmat/backends/openssl/ocsp.py
+++ b/src/cryptography/hazmat/backends/openssl/ocsp.py
@@ -4,23 +4,28 @@
 
 from __future__ import absolute_import, division, print_function
 
-import operator
-
 from cryptography import utils
 from cryptography.exceptions import UnsupportedAlgorithm
 from cryptography.hazmat.backends.openssl.decode_asn1 import (
     _asn1_integer_to_int, _asn1_string_to_bytes, _obj2txt
 )
 from cryptography.hazmat.primitives import serialization
-from cryptography.x509.ocsp import OCSPRequest, Request, _OIDS_TO_HASH
+from cryptography.x509.ocsp import OCSPRequest, _OIDS_TO_HASH
 
 
-@utils.register_interface(Request)
-class _Request(object):
-    def __init__(self, backend, ocsp_request, request):
+@utils.register_interface(OCSPRequest)
+class _OCSPRequest(object):
+    def __init__(self, backend, ocsp_request):
+        if backend._lib.OCSP_request_onereq_count(ocsp_request) > 1:
+            raise NotImplementedError(
+                'OCSP request contains more than one request'
+            )
         self._backend = backend
         self._ocsp_request = ocsp_request
-        self._request = request
+        self._request = self._backend._lib.OCSP_request_onereq_get0(
+            self._ocsp_request, 0
+        )
+        self._backend.openssl_assert(self._request != self._backend._ffi.NULL)
         self._cert_id = self._backend._lib.OCSP_onereq_get0_id(self._request)
         self._backend.openssl_assert(self._cert_id != self._backend._ffi.NULL)
 
@@ -74,23 +79,6 @@ class _Request(object):
                 "Signature algorithm OID: {0} not recognized".format(oid)
             )
 
-
-@utils.register_interface(OCSPRequest)
-class _OCSPRequest(object):
-    def __init__(self, backend, ocsp_request):
-        self._backend = backend
-        self._ocsp_request = ocsp_request
-
-    def __len__(self):
-        return self._backend._lib.OCSP_request_onereq_count(self._ocsp_request)
-
-    def _request(self, idx):
-        request = self._backend._lib.OCSP_request_onereq_get0(
-            self._ocsp_request, idx
-        )
-        self._backend.openssl_assert(request != self._backend._ffi.NULL)
-        return _Request(self._backend, self._ocsp_request, request)
-
     def public_bytes(self, encoding):
         if encoding is not serialization.Encoding.DER:
             raise ValueError(
@@ -101,19 +89,3 @@ class _OCSPRequest(object):
         res = self._backend._lib.i2d_OCSP_REQUEST_bio(bio, self._ocsp_request)
         self._backend.openssl_assert(res > 0)
         return self._backend._read_mem_bio(bio)
-
-    def __iter__(self):
-        for i in range(len(self)):
-            yield self._request(i)
-
-    def __getitem__(self, idx):
-        if isinstance(idx, slice):
-            start, stop, step = idx.indices(len(self))
-            return [self._request(i) for i in range(start, stop, step)]
-        else:
-            idx = operator.index(idx)
-            if idx < 0:
-                idx += len(self)
-            if not 0 <= idx < len(self):
-                raise IndexError
-            return self._request(idx)
diff --git a/src/cryptography/x509/ocsp.py b/src/cryptography/x509/ocsp.py
index 0567197db..c3225daae 100644
--- a/src/cryptography/x509/ocsp.py
+++ b/src/cryptography/x509/ocsp.py
@@ -27,10 +27,13 @@ def load_der_ocsp_request(data):
 
 
 class OCSPRequestBuilder(object):
-    def __init__(self, requests=[]):
-        self._requests = requests
+    def __init__(self, request=None):
+        self._request = request
+
+    def add_certificate(self, cert, issuer, algorithm):
+        if self._request is not None:
+            raise ValueError("Only one certificate can be added to a request")
 
-    def add_request(self, cert, issuer, algorithm):
         allowed_hashes = (
             hashes.SHA1, hashes.SHA224, hashes.SHA256,
             hashes.SHA384, hashes.SHA512
@@ -45,45 +48,18 @@ class OCSPRequestBuilder(object):
         ):
             raise TypeError("cert and issuer must be a Certificate")
 
-        return OCSPRequestBuilder(self._requests + [(cert, issuer, algorithm)])
+        return OCSPRequestBuilder((cert, issuer, algorithm))
 
     def build(self):
         from cryptography.hazmat.backends.openssl.backend import backend
-        if len(self._requests) == 0:
-            raise ValueError("You must add a request before building")
+        if self._request is None:
+            raise ValueError("You must add a certificate before building")
 
         return backend.create_ocsp_request(self)
 
 
 @six.add_metaclass(abc.ABCMeta)
 class OCSPRequest(object):
-    @abc.abstractmethod
-    def __iter__(self):
-        """
-        Iteration of Requests
-        """
-
-    @abc.abstractmethod
-    def __len__(self):
-        """
-        Number of Requests inside the OCSPRequest object
-        """
-
-    @abc.abstractmethod
-    def __getitem__(self, idx):
-        """
-        Returns a Request or range of Requests
-        """
-
-    @abc.abstractmethod
-    def public_bytes(self, encoding):
-        """
-        Serializes the request to DER
-        """
-
-
-@six.add_metaclass(abc.ABCMeta)
-class Request(object):
     @abc.abstractproperty
     def issuer_key_hash(self):
         """
@@ -107,3 +83,8 @@ class Request(object):
         """
         The serial number of the cert whose status is being checked
         """
+    @abc.abstractmethod
+    def public_bytes(self, encoding):
+        """
+        Serializes the request to DER
+        """
diff --git a/tests/x509/test_ocsp.py b/tests/x509/test_ocsp.py
index 709ef6f49..3e6ac9cde 100644
--- a/tests/x509/test_ocsp.py
+++ b/tests/x509/test_ocsp.py
@@ -46,64 +46,26 @@ class TestOCSPRequest(object):
         with pytest.raises(ValueError):
             ocsp.load_der_ocsp_request(b"invalid")
 
-    def test_load_request_one_item(self):
+    def test_load_request(self):
         req = _load_data(
             os.path.join("x509", "ocsp", "req-sha1.der"),
             ocsp.load_der_ocsp_request,
         )
-        assert len(req) == 1
-        assert req[0].issuer_name_hash == (b"8\xcaF\x8c\x07D\x8d\xf4\x81\x96"
-                                           b"\xc7mmLpQ\x9e`\xa7\xbd")
-        assert req[0].issuer_key_hash == (b"yu\xbb\x84:\xcb,\xdez\t\xbe1"
-                                          b"\x1bC\xbc\x1c*MSX")
-        assert isinstance(req[0].hash_algorithm, hashes.SHA1)
-        assert req[0].serial_number == int(
+        assert req.issuer_name_hash == (b"8\xcaF\x8c\x07D\x8d\xf4\x81\x96"
+                                        b"\xc7mmLpQ\x9e`\xa7\xbd")
+        assert req.issuer_key_hash == (b"yu\xbb\x84:\xcb,\xdez\t\xbe1"
+                                       b"\x1bC\xbc\x1c*MSX")
+        assert isinstance(req.hash_algorithm, hashes.SHA1)
+        assert req.serial_number == int(
             "98D9E5C0B4C373552DF77C5D0F1EB5128E4945F9", 16
         )
 
-    def test_load_request_multiple_items(self):
-        req = _load_data(
-            os.path.join("x509", "ocsp", "req-multi-sha1.der"),
-            ocsp.load_der_ocsp_request,
-        )
-        assert len(req) == 2
-        assert req[0].issuer_name_hash == (b"8\xcaF\x8c\x07D\x8d\xf4\x81\x96"
-                                           b"\xc7mmLpQ\x9e`\xa7\xbd")
-        assert req[0].issuer_key_hash == (b"yu\xbb\x84:\xcb,\xdez\t\xbe1"
-                                          b"\x1bC\xbc\x1c*MSX")
-        assert isinstance(req[0].hash_algorithm, hashes.SHA1)
-        assert req[0].serial_number == int(
-            "98D9E5C0B4C373552DF77C5D0F1EB5128E4945F9", 16
-        )
-        assert req[1].issuer_name_hash == (b"8\xcaF\x8c\x07D\x8d\xf4\x81\x96"
-                                           b"\xc7mmLpQ\x9e`\xa7\xbd")
-        assert req[1].issuer_key_hash == (b"yu\xbb\x84:\xcb,\xdez\t\xbe1"
-                                          b"\x1bC\xbc\x1c*MSX")
-        assert isinstance(req[1].hash_algorithm, hashes.SHA1)
-        assert req[1].serial_number == int(
-            "98D9E5C0B4C373552DF77C5D0F1EB5128E4945F0", 16
-        )
-
-    def test_iter(self):
-        req = _load_data(
-            os.path.join("x509", "ocsp", "req-multi-sha1.der"),
-            ocsp.load_der_ocsp_request,
-        )
-        for request in req:
-            assert isinstance(request, ocsp.Request)
-
-    def test_indexing_ocsp_request(self):
-        req = _load_data(
-            os.path.join("x509", "ocsp", "req-multi-sha1.der"),
-            ocsp.load_der_ocsp_request,
-        )
-        assert req[1].serial_number == req[-1].serial_number
-        assert len(req[0:2]) == 2
-        assert req[1:2][0].serial_number == int(
-            "98D9E5C0B4C373552DF77C5D0F1EB5128E4945F0", 16
-        )
-        with pytest.raises(IndexError):
-            req[10]
+    def test_load_request_two_requests(self):
+        with pytest.raises(NotImplementedError):
+            _load_data(
+                os.path.join("x509", "ocsp", "req-multi-sha1.der"),
+                ocsp.load_der_ocsp_request,
+            )
 
     def test_invalid_hash_algorithm(self):
         req = _load_data(
@@ -111,7 +73,7 @@ class TestOCSPRequest(object):
             ocsp.load_der_ocsp_request,
         )
         with pytest.raises(UnsupportedAlgorithm):
-            req[0].hash_algorithm
+            req.hash_algorithm
 
     def test_serialize_request(self):
         req_bytes = load_vectors_from_file(
@@ -134,6 +96,13 @@ class TestOCSPRequest(object):
 
 
 class TestOCSPRequestBuilder(object):
+    def test_add_two_certs(self):
+        cert, issuer = _cert_and_issuer()
+        builder = ocsp.OCSPRequestBuilder()
+        builder = builder.add_certificate(cert, issuer, hashes.SHA1())
+        with pytest.raises(ValueError):
+            builder.add_certificate(cert, issuer, hashes.SHA1())
+
     def test_create_ocsp_request_no_req(self):
         builder = ocsp.OCSPRequestBuilder()
         with pytest.raises(ValueError):
@@ -143,37 +112,24 @@ class TestOCSPRequestBuilder(object):
         cert, issuer = _cert_and_issuer()
         builder = ocsp.OCSPRequestBuilder()
         with pytest.raises(ValueError):
-            builder.add_request(cert, issuer, hashes.MD5())
+            builder.add_certificate(cert, issuer, hashes.MD5())
 
     def test_create_ocsp_request_invalid_cert(self):
         cert, issuer = _cert_and_issuer()
         builder = ocsp.OCSPRequestBuilder()
         with pytest.raises(TypeError):
-            builder.add_request(b"notacert", issuer, hashes.SHA1())
+            builder.add_certificate(b"notacert", issuer, hashes.SHA1())
 
         with pytest.raises(TypeError):
-            builder.add_request(cert, b"notacert", hashes.SHA1())
+            builder.add_certificate(cert, b"notacert", hashes.SHA1())
 
     def test_create_ocsp_request(self):
         cert, issuer = _cert_and_issuer()
         builder = ocsp.OCSPRequestBuilder()
-        builder = builder.add_request(cert, issuer, hashes.SHA1())
+        builder = builder.add_certificate(cert, issuer, hashes.SHA1())
         req = builder.build()
         serialized = req.public_bytes(serialization.Encoding.DER)
         assert serialized == base64.b64decode(
             b"MEMwQTA/MD0wOzAJBgUrDgMCGgUABBRAC0Z68eay0wmDug1gfn5ZN0gkxAQUw5zz"
             b"/NNGCDS7zkZ/oHxb8+IIy1kCAj8g"
         )
-
-    def test_create_ocsp_request_two_reqs(self):
-        builder = ocsp.OCSPRequestBuilder()
-        cert, issuer = _cert_and_issuer()
-        builder = builder.add_request(cert, issuer, hashes.SHA1())
-        builder = builder.add_request(cert, issuer, hashes.SHA1())
-        req = builder.build()
-        serialized = req.public_bytes(serialization.Encoding.DER)
-        assert serialized == base64.b64decode(
-            b"MIGDMIGAMH4wPTA7MAkGBSsOAwIaBQAEFEALRnrx5rLTCYO6DWB+flk3SCTEBBTD"
-            b"nPP800YINLvORn+gfFvz4gjLWQICPyAwPTA7MAkGBSsOAwIaBQAEFEALRnrx5rLT"
-            b"CYO6DWB+flk3SCTEBBTDnPP800YINLvORn+gfFvz4gjLWQICPyA="
-        )