On Thursday 23 February 2006 21:40, Simon Wilkinson wrote:
> Achim,
>
> I'm having some trouble understanding the requirements for these
> changes.
That's my fault because I have sent my personal working copy.
Please don't feel offended by me, I have sent my code just
as a base of discussion.
Please let us discuss now, see my comments.
> Some of this may be because of changes you've made to the
> GSSAPI.pm perl module, since Phillip Guenther released version 0.13 in
> 2005.
I have not touched the modules interface.
I don't want to break other peoples code.
Please let me know what problems you have with my changes
to Philip Guenthers module, I will try to fix that.
> Also, there seem to be a lot of purely formatting changes, which make it
> harder to see where the functional differences lie.
My fault, too.
> Some comments below
> follow ...
>
> > -use GSSAPI;
> > +use GSSAPI qw(:all);
>
> Is symbol exporting from GSSAPI.pm now controlled? This change breaks
> compatibility with GSSAPI v0.13.
use GSSAPI will work.
> > + my $target;
> > + $status = GSSAPI::Name->import( $target,
> > + $principal,
> > + GSSAPI::OID::gss_nt_hostbased_service );
>
> This change breaks compatibility with GSSAPI 0.13, which doesn't provide
> gss_nt_hostbased_service.
gss_nt_hostbased_service is exported *additionally*.
feel free to use gss_nt_service_name.
> > + if ( $status->major == GSS_S_COMPLETE ) {
> > + $self->{gss_ctx} = new GSSAPI::Context;
> > + $self->{gss_state} = 0;
> > - $self->{gss_name} = new GSSAPI::Name;
> > - $status = $self->{gss_name}->import($self->{gss_name}, $principal,
> > - gss_nt_service_name)
> > - or return $self->set_error("GSSAPI Error : ".$status);
>
> I believe that the existing code provides sufficient error checking on
> the return from import().
Yes.
but the ->import() method has to be used as Class method like
GSSAPI::Name->import( target,
principal,
gss_nt_service_name )
> > - $self->{gss_ctx} = new GSSAPI::Context;
> > - $self->{gss_state} = 0;
> > - return $self->client_step("");
> > + my $tname;
> > + $status = $target->display($tname);
> > + if ( $status->major != GSS_S_COMPLETE ) {die "GSSAPI Error :
> > ".$status; }
>
> I don't think that we should be die()ing from the middle of the SASL
> library,
Correct.
I have written in alle my Emails that the 'die' has to be replaced
with somthing better before commiting it to the SVN.
The 'die' was only for my testing.
> especially given that successfully obtaining a displayname
> isn't required for the success of the SASL handshake.
The displayname is interesting for debugging if you want to see
what hostname is used in the GSSAPI token.
> > + __debug_message( " Name import OK $tname");
> > + $self->{gss_name} = $target;
> > + return $self->client_step( q{} );
> > + }
> > + else {
> > + die 'GSSAPI name import error ', $self->host, ' ', $status;
> > + return $self->set_error("GSSAPI Error : ".$status);
>
> Again, I think that set_error should be allowed to return the error case
> up to the caller, rather than calling die().
Yes.
I have written in alle my Emails thet the 'die' has to be replaced
with somthing better before commiting it to the SVN.
The die was only for my testing.
> > - my $outtok;
> > - my $flags;
> > - $status = $self->{gss_ctx}->init(undef, $self->{gss_name},
> > - gss_mech_krb5,
> > - GSS_C_INTEG_FLAG | GSS_C_MUTUAL_FLAG,
> > - undef, undef, $challenge, undef,
> > - $outtok, $flags, undef);
> >
> > - if (GSSAPI::Status::GSS_ERROR($status->major)) {
> > - return $self->set_error("GSSAPI Error : ".$status);
> > - }
> > - if ($status->major == GSS_S_COMPLETE) {
> > - $self->{gss_state}=1;
> > - }
> > - return $outtok;
> > + my $itime = 0;
> > + my $bindings = GSS_C_NO_CHANNEL_BINDINGS;
> > + my $imech = GSSAPI::OID::gss_mech_krb5;
> > +
> > + my $iflags = GSS_C_MUTUAL_FLAG | GSS_C_INTEG_FLAG ;
> > + my $creds = GSS_C_NO_CREDENTIAL;
> > + my ( $omech,$otoken,$oflags,$otime);
> > +
> > + $status = $self->{gss_ctx}->init( $creds,
> > + $self->{gss_name} ,
> > + $imech, $iflags, $itime,
> > $bindings, $challenge, +
> > $omech,$otoken,$oflags,$otime); +
>
> Why are you allocating return variables for paramaters that we're not
> interested in?
Just copypasting. My fault.
> Does your new GSSAPI.pm no longer permit passing 'undef'
> for variables that the caller doesn't care about?
You can pass undef.
> Also, 'undef' (or NULL) should be equivalent to GSS_C_NO_CREDENTIAL and
> GSS_C_NO_CHANNEL_BINDINGS, but I accept that using those definitions
> might improve clarity.
Right.
In this case I have inserted them for clarity.
> > - $status = $self->{gss_ctx}->unwrap($challenge, $unwrapped, undef,
> > undef) - or return $self->set_error("GSSAPI Error : ".$status);
> > + $status = $self->{gss_ctx}->unwrap( $challenge,
> > + $unwrapped,
> > + undef,
> > + undef);
> > + if ( $status->major != GSS_S_COMPLETE ) { die 'unwrap error',
> > $status; }
>
> These are functionally equivalent, are they not (with the exception of
> the 'die' vs 'seterror' discussed above?)
They are equivalent.
>
> > - $message.= $self->_call('user');
> > + $message.= $self->_call('user') if ( $self->_call('user') );
>
> Yes - we should check that they've actually defined a user.
>
> > +
> > my $outtok;
> > - $status = $self->{gss_ctx}->wrap(0, undef, $message, undef, $outtok)
> > - or return $self->set_error("GSSAPI Error : ".$status);
> > -
> > + $status = $self->{gss_ctx}->wrap( 0,
> > + 0,
> > + $message,
> > + 0,
> > + $outtok,
> > + );
>
> The second parameter you've changed here should probably remain 'undef',
> as its a output variable.
conf_state?
Correct. My fault.
> > + if ( $status->major != GSS_S_COMPLETE ) { die 'wrap error', $status;
> > }
>
> You don't need to check $status->major, a boolean check on $status works
> according to the GSSAPI::Status documentation.
>
> In general, I think that the error reporting problem should probably be
> fixed by changing Net::LDAP to expose the SASL error state to the caller
> of bind(), rather than by die()ing in this particular module.
Correct.
I have written in alle my Emails thet the 'die' has to be replaced
with somthing better before commiting it to the SVN.
The 'die' was only for my testing.
Thank you for reading my code, thank you for pointing on my errors.
What comes next?
Simon,
Do you want me to send a new patch with my minimal changes that includes all
the things you wrote in this email?
Thank you,
Achim
