Achim,
I'm having some trouble understanding the requirements for these
changes. 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.
Also, there seem to be a lot of purely formatting changes, which make it
harder to see where the functional differences lie. 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.
> + 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.
> + 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().
> - $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, especially given that successfully obtaining a displayname
isn't required for the success of the SASL handshake.
> + __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().
> - 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? Does your new GSSAPI.pm no longer permit passing 'undef'
for variables that the caller doesn't care about?
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.
> - $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?)
> - $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.
> + 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.
Cheers,
Simon.
