• 
      

    Add a stunnel mode for Perforce.

    Review Request #2482 — Created July 15, 2011 and submitted

    Information

    Review Board

    Reviewers

    Add a stunnel mode for Perforce.
    
    Out of the box, Perforce doesn't support any sort of secure connectivity. This
    is an artifact of the sorts of environment that Perforce is usually run in, but
    isn't really a modern approach to life (people want security even inside the
    firewall).
    
    The suggested workaround for this is to run a stunnel proxy on the server, and a
    stunnel client proxy on the client. Users can then connect using a localhost:
    P4PORT and the encryption will be handled by stunnel.
    
    This change adds support for this configuration, by adding a special stunnel:
    prefix to the P4PORT setting. When someone uses this prefix, the port will be
    interpreted as a stunnel server to connect to. We then spin up a stunnel client
    on a random port and use that as the P4PORT. When we're done, we disconnect
    perforce and kill the stunnel process.
    
    This is a little complicated in implementation because stunnel can either run in
    the foreground (in which case we race to see whether we try to connect before it
    listens on the local port), or it can run in the background (in which case we
    race the -filesystem- for when it writes out its PID file). I've chosen the
    latter case as the lesser of the two races.
    - Unit tests
    - Tested setting up a stunnel server and adding that repository via
      stunnel:localhost:port, and posted a review request to it. The changeset
      contents and diff loaded just fine.
    Description From Last Updated

    No need for this. You'll get it for free from os.

    chipx86 chipx86

    Can you make this inherit from object?

    chipx86 chipx86

    We should probably bullet-proof this a bit in case the contents of stunnel.pid can't be converted into an int. Probably …

    chipx86 chipx86

    I don't see found ever becoming True. Just while 1? Do we want to risk this somehow infinite looping? Maybe …

    chipx86 chipx86

    Should inherit from object.

    chipx86 chipx86

    Too many blank lines.

    chipx86 chipx86

    The blank line isn't really needed.

    chipx86 chipx86

    Shouldn't we pass use_stunnel?

    chipx86 chipx86

    To specify a password in the p4 command line, you should use the upper case -P option, not the lower …

    FA Faller.Gyula
    chipx86
    1. Is this going in for 1.6? Any worry about regression?
      1. I'm hoping to do it for 1.6. I'm pretty confident based on the unit tests, but I'll do some additional testing (especially with check_repository).
    2. reviewboard/scmtools/perforce.py (Diff revision 1)
       
       
      Show all issues
      No need for this. You'll get it for free from os.
    3. reviewboard/scmtools/perforce.py (Diff revision 1)
       
       
      Show all issues
      Can you make this inherit from object?
    4. reviewboard/scmtools/perforce.py (Diff revision 1)
       
       
       
       
       
       
      Not really a big deal, but I noticed this block is repeated twice (except for the '-c' and '-p', certfile). Is it worth consolidating some of this? A run_stunnel with provided args?
      1. I already have a _start which deals with args, so I'll move the common stuff into there.
    5. reviewboard/scmtools/perforce.py (Diff revision 1)
       
       
      Show all issues
      We should probably bullet-proof this a bit in case the contents of stunnel.pid can't be converted into an int. Probably just catch ValueError and bail somehow?
      1. I think we can actually let ValueError propagate and it'll just show up as an error in whatever operation you called. Either way things are less than ideal because we might leak a stunnel process.
    6. reviewboard/scmtools/perforce.py (Diff revision 1)
       
       
       
      Show all issues
      I don't see found ever becoming True. Just while 1? Do we want to risk this somehow infinite looping? Maybe try a max number of times?
      1. Oh, it's left over from before I added "return".
        
        If there's no available ports in a range of 30000 (or a small number of available ports) we're in serious trouble anyway, and I don't really care to work around that case.
    7. reviewboard/scmtools/perforce.py (Diff revision 1)
       
       
      Show all issues
      Should inherit from object.
    8. reviewboard/scmtools/perforce.py (Diff revision 1)
       
       
       
       
       
       
      Show all issues
      Too many blank lines.
    9. reviewboard/scmtools/perforce.py (Diff revision 1)
       
       
       
       
       
       
       
       
       
       
      We do this in multiple places. Can we consolidate?
    10. reviewboard/scmtools/perforce.py (Diff revision 1)
       
       
       
       
      Show all issues
      The blank line isn't really needed.
    11. reviewboard/scmtools/perforce.py (Diff revision 1)
       
       
      Show all issues
      Shouldn't we pass use_stunnel?
      1. Yep. Fixed in the consolidation above.
    12. 
        
    david
    david
    chipx86
    1. Ship It!
    2. 
        
    david
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to master (6d4bc92).
    FA
    1. Found a serious typo.
    2. reviewboard/scmtools/perforce.py (Diff revision 3)
       
       
      Show all issues
      To specify a password in the p4 command line, you should use the upper case -P option, not the lower case -p, which means the port instead. (See the line 131 in the original file.)
      This is a serious regression bug since make it impossible to use ReviewBoard with Perforce if you need to supply a password.
      I suggest to correct it in the final 1.6.
    3.