[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Bug#139710: first version of https support available



Thanks heaps for commenting Jason!

(Should I be Cc: all those lists?
(Below: Ack == Yes, I'll do that)

On Tue, 2 Apr 2002, Jason Gunthorpe wrote:

> On Tue, 2 Apr 2002, Tomas Pospisek wrote:
>
> > http: //sourcepole.ch/sources/software/apt_https/
>
> > * how do you like the integration with the rest of your stuff?
>
> Going down the diff..
>
> +ah_WITH_SSL
>
> You are not 'ah', use to_ or something..

Ack, thanks (autotools newbie here)

> The macro does not seem to actually test if SSL is available and working
> to build against, it probably should.

Ack. Fixed. As of now I'm testing for -lssl and for openssl/ssl.h. And
just failing and telling the user to install it somewhere where configure
can find it. If that's not traditional, expected behaveour I'd appreciate
a hint.

> All the changes to connect.cc appear to be needless.

Ack.

> +#ifdef WITH_SSL
> +bool CircleBuf::Read(SSL * Ssl)
>
> This is C++. The way to implement this without wacks of #ifdefs everywere
> is to suck a few more things into circlebuf and subclass it for the SSL
> case. The wacks of ifdefs and 'if https's are really excessive.

I sympathize. My motive for using #ifdefs was to keep the executable
SSL-clean, so that it's possible to link it without libssl.

I can see that I can subclass "class CircleBufSSL : CircleBuf". Should I
put that into a separate file or leave that in http.*?

But what about the other classes? SSL-enabling f.ex. ServerState means
just minimal changements to ServerState. If I subclass it I will have to
override some methods and copy-paste 95% of their code over to the new
method (f.ex. in ServerState::Open), which will result in two nearly
identical code sets that need to be maintained. Is this what you want me
to do? (IMHO not a good idea). See further below for more about the issue.

> +   string ProxyStr;
>
> Making 'ProxyStr' a string is silly, it is never assigned to anything but
> a constant.

Ack, thanks.

> ConnectSSL really needs to use the normal connect function and then just
> bind the SSL library to that FD (there are functions in openssl to do
> this). The normal connect function does much more than what openssl
> provides.

I'm not sure I understand you correctly - this is exactly what's happening
right now - except that it's in ServerState::Open where this is happening
(ifdefs and logging removed):

if (ServerName.Access == "https") {
      if (Connect(Host,Port,"https",443,ServerFd,TimeOut,Owner) == false)
         return false;
      else if (ConnectSSL(ServerFd,&Ssl))
         return false;
      return false;
   }
   else if (Connect(Host,Port,"http",80,ServerFd,TimeOut,Owner) == false)
      return false;

We first do Connect through the allready existing mechanism and bind then
SSL on top of it. Maybe you want me to move this *into* ConnectSSL, which
I can do. Or alternatively I can:

   if (ServerName.Access == "https")
   {
      ProxyStr = "https_proxy";
      Acquire = "Acquire::https::Proxy";
      DefaultPort = 443;
      Protocol = "https";
   }
   else
   {
      ProxyStr = "http_proxy";
      Acquire = "Acquire::http::Proxy";
      DefaultPort = 80;
      Protocol = "http";
   }

   if (Connect(Host,Port,Protocol,DefaultPort,ServerFd,TimeOut,Owner) == false)
         return false;
   if(Protocol == "https" && ConnectSSL(ServerFd,&Ssl)
         return false;

Looks cleaner anyway :-), let me know.

> Ultimately I don't think we can expect to have https included with APT, as
> that needs openssl on the base install.

Ack. That was my intention.

(OT: Btw. don't you think basic encryption algorithms will move into base
with time? Most networking programs will be SSL enabled one day anyway).

> So at best it would have to compile seperately to something called https
> (this is trivial).

Ack.

> You also
> forgot the makefile bit to actually make the https symlink, which would be
> necessary for this to work at all.

Ack.

> > Certificate validation is on my todo list.
>
> SSL is utterly pointless without this,

Well at least you can not hijack the connection any more and the transfer
is unreadable. But Ack all the same!

> and I'm not sure how you would go about implementing it in this sort of
> context..

I do want to implement it - I'll see! My intention with the posting
was to see if the apt team was interested at all in my contribution and to
make sure it fits in before I proceed implementing.
*t

-----------------------------------------------------------------------
     Tomas Pospisek
	 Infinite Justice for the World:
	 http://www.heise.de/tp/deutsch/inhalt/co/11621/11621_2.jpg
------------------------------------------------------------------------


-- 
To UNSUBSCRIBE, email to deity-request@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org



Reply to: