Add a sense of purpose to SSL/TLS certificates.

 






 

 


  Review Request #14896
  —
  Created March 11, 2026 and updated 

 




















 

  

   Information

  





 
 

  chipx86
 









 
 

  Review Board
 









 
 

  release-7.1.x
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
My original design for SSL/TLS certificate management combined two

important purposes into one concept of a certificate. Depending on the

call site and the presence of a private key, a Certificate could be

used for verifying a server or authenticating the client with a server.

These are very different purposes, and needed to be managed separately.


The original design led to incorrect calls in build_ssl_context() and

therefore build_urlopen_kwargs(), meaning we didn't always have the

right context in place to handle the operation.


This change addresses this by:


    

  1. 

    Introducing a CertPurpose enum containing TRUST (for
    
   cert trust/verification needs) and CLIENT (for client
    
   authentication).
    

    2. 

  2. 

    A purpose field in Certificate and all cert-related operations in
    
CertificateManager, and storage equivalents in the backends.
    

    3. 



A "trust" cert may not have a private key, and a "client" cert must

(although technically that cert could embed the key, we're requiring two

separate files in our implementation for now).


This does change the signatures and expectations in the storage

backends, and we're not going through a deprecation cycle for that. The

reason being that, while we introduced all this in Review Board 6, we

never made actual use of it. Going through a deprecation cycle would

introduce a fair amount of complexity, and isn't worth it for something

that users haven't touched or would have had a need to build upon.


One of the backwards-incompatible changes is the location of files in

the file storage backend. Both kinds of certs used to live in certs/,

but now they live in certs/client/ and certs/purpose/ instead.


The change is pretty straight-forward. The biggest change in behavior is

build_ssl_context(), which now passes the right kinds of certs to the

right functions.


The bulk of the change is actually unit tests and testdata file

movement. We now have trust and client purpose variations for each

relevant test.


As a note, there's some initial attempts at modernizing some of the

typing in this change, but it's purposefully incomplete. The rest will

be tackled separately.

 








  


 
 

  
All unit tests pass.

 







 









 





  

 


 
 

  



 

  
   


    Summary
    ID

   

  
  

   


    

     

      Add a sense of purpose to SSL/TLS certificates.
      
My original design for SSL/TLS certificate management combined two
important purposes into one concept of a certificate. Depending on the
call site and the presence of a private key, a `Certificate` could be
used for verifying a server or authenticating the client with a server.
These are very different purposes, and needed to be managed separately.

The original design led to incorrect calls in `build_ssl_context()` and
therefore `build_urlopen_kwargs()`, meaning we didn't always have the
right context in place to handle the operation.

This change addresses this by:

1. Introducing a `CertPurpose` enum containing `TRUST` (for
   cert trust/verification needs) and `CLIENT` (for client
   authentication).

2. A `purpose` field in `Certificate` and all cert-related operations in
   `CertificateManager`, and storage equivalents in the backends.

A "trust" cert may not have a private key, and a "client" cert must
(although technically that cert could embed the key, we're requiring two
separate files in our implementation for now).

This does change the signatures and expectations in the storage
backends, and we're not going through a deprecation cycle for that. The
reason being that, while we introduced all this in Review Board 6, we
never made actual use of it. Going through a deprecation cycle would
introduce a fair amount of complexity, and isn't worth it for something
that users haven't touched or would have had a need to build upon.

One of the backwards-incompatible changes is the location of files in
the file storage backend. Both kinds of certs used to live in `certs/`,
but now they live in `certs/client/` and `certs/purpose/` instead.

The change is pretty straight-forward. The biggest change in behavior is
`build_ssl_context()`, which now passes the right kinds of certs to the
right functions.

The bulk of the change is actually unit tests and testdata file
movement. We now have trust and client purpose variations for each
relevant test.

     


    		
    df8e4659369631fe00d7778df9f6890bd91781b3

   



  
 




 













 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
Your description says certs/purpose/ where I think it should say certs/trust/

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
'typing.cast' imported but unused Column: 1 Error code: F401

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
too many blank lines (2) Column: 5 Error code: E303

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
local variable 'local_site1' is assigned to but never used Column: 9 Error code: F841

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
This should use typelets.runtime.raise_invalid_type() so type checkers don't flag this branch as unreachable.

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
It seems like this should happen before we validate key_path

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
This should use typelets.runtime.raise_invalid_type() so type checkers don't flag this branch as unreachable.

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
Since this class is already an ABC subclass, perhaps we should wrap this method in @abstractmethod, so that it's not …

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
This doesn't seem right. _build_fingerprints_file_path doesn't take a purpose argument, and should still return <hostname>__<port>.json .

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
This isn't a new issue with the change, but a bunch of the docstrings here say create_form_files instead of create_from_files

      		
      
       daviddavid
      
      
       
      
     


    
   

  

 







 






  

 



















 




 

  


   

   

    
 
Checks run (1 failed, 1 succeeded)


    
   


  

  

  





 

 flake8
 failed.
 





 

 JSHint
 passed.
 













 
flake8

 


 










  

 





 














 



 

  

   




chipx86




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  









  

   

    Change Summary:
   

   

    
Removed an unused import, an unused variable, and a blank line.

   

  








  

   

    Commits:
   

   

    





 

  
   


    

    Summary
    ID

   

  
  

   


    

    

     

      Add a sense of purpose to SSL/TLS certificates.
      
My original design for SSL/TLS certificate management combined two
important purposes into one concept of a certificate. Depending on the
call site and the presence of a private key, a `Certificate` could be
used for verifying a server or authenticating the client with a server.
These are very different purposes, and needed to be managed separately.

The original design led to incorrect calls in `build_ssl_context()` and
therefore `build_urlopen_kwargs()`, meaning we didn't always have the
right context in place to handle the operation.

This change addresses this by:

1. Introducing a `CertPurpose` enum containing `TRUST` (for
   cert trust/verification needs) and `CLIENT` (for client
   authentication).

2. A `purpose` field in `Certificate` and all cert-related operations in
   `CertificateManager`, and storage equivalents in the backends.

A "trust" cert may not have a private key, and a "client" cert must
(although technically that cert could embed the key, we're requiring two
separate files in our implementation for now).

This does change the signatures and expectations in the storage
backends, and we're not going through a deprecation cycle for that. The
reason being that, while we introduced all this in Review Board 6, we
never made actual use of it. Going through a deprecation cycle would
introduce a fair amount of complexity, and isn't worth it for something
that users haven't touched or would have had a need to build upon.

One of the backwards-incompatible changes is the location of files in
the file storage backend. Both kinds of certs used to live in `certs/`,
but now they live in `certs/client/` and `certs/purpose/` instead.

The change is pretty straight-forward. The biggest change in behavior is
`build_ssl_context()`, which now passes the right kinds of certs to the
right functions.

The bulk of the change is actually unit tests and testdata file
movement. We now have trust and client purpose variations for each
relevant test.

     


    		
    da5fa26364cb96ef5e3380c4769a5f3083287366

   




   


    

    

     

      Add a sense of purpose to SSL/TLS certificates.
      
My original design for SSL/TLS certificate management combined two
important purposes into one concept of a certificate. Depending on the
call site and the presence of a private key, a `Certificate` could be
used for verifying a server or authenticating the client with a server.
These are very different purposes, and needed to be managed separately.

The original design led to incorrect calls in `build_ssl_context()` and
therefore `build_urlopen_kwargs()`, meaning we didn't always have the
right context in place to handle the operation.

This change addresses this by:

1. Introducing a `CertPurpose` enum containing `TRUST` (for
   cert trust/verification needs) and `CLIENT` (for client
   authentication).

2. A `purpose` field in `Certificate` and all cert-related operations in
   `CertificateManager`, and storage equivalents in the backends.

A "trust" cert may not have a private key, and a "client" cert must
(although technically that cert could embed the key, we're requiring two
separate files in our implementation for now).

This does change the signatures and expectations in the storage
backends, and we're not going through a deprecation cycle for that. The
reason being that, while we introduced all this in Review Board 6, we
never made actual use of it. Going through a deprecation cycle would
introduce a fair amount of complexity, and isn't worth it for something
that users haven't touched or would have had a need to build upon.

One of the backwards-incompatible changes is the location of files in
the file storage backend. Both kinds of certs used to live in `certs/`,
but now they live in `certs/client/` and `certs/purpose/` instead.

The change is pretty straight-forward. The biggest change in behavior is
`build_ssl_context()`, which now passes the right kinds of certs to the
right functions.

The bulk of the change is actually unit tests and testdata file
movement. We now have trust and client purpose variations for each
relevant test.

     


    		
    df8e4659369631fe00d7778df9f6890bd91781b3

   



  
 







   

  





  

   

    Diff:
   

   

    
 Revision 2 (+5444 -1576)
Show changes
 
    reviewboard/certs/cert.py 
    reviewboard/certs/manager.py 
    reviewboard/certs/storage/base.py 
    reviewboard/certs/storage/file_storage.py 
    reviewboard/certs/tests/test_certificate.py 
    reviewboard/certs/tests/test_certificate_fingerprints.py 
    reviewboard/certs/tests/test_certificate_manager.py 
    reviewboard/certs/tests/test_file_storage_backend.py 
  18 more
 

   

  











 

  
Checks run (2 succeeded)



  

 flake8
 passed.
 





  

 JSHint
 passed.
 





 















  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david



    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 







  2. 
 
    
  
  
    
  
   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Your description says certs/purpose/ where I think it should say certs/trust/
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 















  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/certs/cert.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This should use typelets.runtime.raise_invalid_type() so type checkers don't flag this branch as unreachable.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/certs/cert.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    It seems like this should happen before we validate key_path
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/certs/cert.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This should use typelets.runtime.raise_invalid_type() so type checkers don't flag this branch as unreachable.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/certs/storage/base.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Since this class is already an ABC subclass, perhaps we should wrap this method in @abstractmethod, so that it's not even possible to define a subclass that doesn't implement this?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/certs/storage/file_storage.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This doesn't seem right. _build_fingerprints_file_path doesn't take a purpose argument, and should still return <hostname>__<port>.json .
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 






  8. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/certs/tests/test_certificate.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This isn't a new issue with the change, but a bunch of the docstrings here say create_form_files instead of create_from_files
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    9. 




  9. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    10.