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.

chipx86chipx86

Can you make this inherit from object?

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Should inherit from object.

chipx86chipx86

Too many blank lines.

chipx86chipx86

The blank line isn't really needed.

chipx86chipx86

Shouldn't we pass use_stunnel?

chipx86chipx86

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.