[PATCH] net: Add support for a credentials file

[Olly Betts via samba-technical]

The attached patch extends net to support the same -A
authfile/--authentication-file authfile option that most of the other
tools already do.

Patch is against current git master.

Please review.

Cheers,
Olly

[Andrew Bartlett via samba-technical]

On Tue, 2018-05-01 at 03:14 +0100, Olly Betts via samba-technical

Post by Olly Betts via samba-technical
The attached patch extends net to support the same -A
authfile/--authentication-file authfile option that most of the other
tools already do.
Patch is against current git master.
Please review.

Thanks Olly,

I like the concept, but the addition of another parser for the file
worries me.

It would be better if it could either wrap or be wrapped by
cli_credentials_parse_file(). Ideally by adding
POPT_COMMON_CREDENTIALS to the popt table and reworking the code to use
it, just like client.c (for smbclient) does.

I realise this is a lot more work, but we need to unify this rather
than see further parser proliferation.

Thanks,

Andrew Bartlett

[Olly Betts via samba-technical]

Post by Andrew Bartlett via samba-technical
I like the concept, but the addition of another parser for the file
worries me.

I went that route as it made for a smaller patch and the format doesn't
seem likely to evolve, but it's not ideal (it's actually just a slightly
adapted version of the parser from cli_credentials_parse_file(), though
that doesn't really make it better).

Post by Andrew Bartlett via samba-technical
It would be better if it could either wrap or be wrapped by
cli_credentials_parse_file(). Ideally by adding
POPT_COMMON_CREDENTIALS to the popt table and reworking the code to use
it, just like client.c (for smbclient) does.

I did look at using POPT_COMMON_CREDENTIALS but there are short
option clashes with existing net options (-N and -S) and would
require doing something with the options in POPT_COMMON_CREDENTIALS
which net doesn't currently support.

Post by Andrew Bartlett via samba-technical
I realise this is a lot more work, but we need to unify this rather
than see further parser proliferation.

Given the concept's liked, I think a common parser shouldn't be too much
work (especially as I forked the existing one).

Cheers,
Olly

[Andrew Bartlett via samba-technical]

On Tue, 2018-05-01 at 04:15 +0100, Olly Betts via samba-technical

Post by Olly Betts via samba-technical

Post by Andrew Bartlett via samba-technical
I like the concept, but the addition of another parser for the file
worries me.

I went that route as it made for a smaller patch and the format doesn't
seem likely to evolve, but it's not ideal (it's actually just a slightly
adapted version of the parser from cli_credentials_parse_file(), though
that doesn't really make it better).

Post by Andrew Bartlett via samba-technical
It would be better if it could either wrap or be wrapped by
cli_credentials_parse_file(). Ideally by adding
POPT_COMMON_CREDENTIALS to the popt table and reworking the code to use
it, just like client.c (for smbclient) does.

I did look at using POPT_COMMON_CREDENTIALS but there are short
option clashes with existing net options (-N and -S) and would
require doing something with the options in POPT_COMMON_CREDENTIALS
which net doesn't currently support.

-N we can probably work around (only used for groupmap). -S is harder,
perhaps removing that from the popt_common.c side would be best (and
fix source4/lib/cmdline/popt_credentials.c to match).

I'm hesitant about breaking scripts but making this use the common
credentials parsing code is really important (not just for parsing the
file, but for the rest of the things it gives us).

Post by Olly Betts via samba-technical

Post by Andrew Bartlett via samba-technical
I realise this is a lot more work, but we need to unify this rather
than see further parser proliferation.

Given the concept's liked, I think a common parser shouldn't be too much
work (especially as I forked the existing one).

If you could give using the POPT_COMMON_CREDENTIALS one more try I
would appreciate it. There is a lot we gain if we unify this.

Thanks,

Andrew Bartlett

[Olly Betts via samba-technical]

Post by Andrew Bartlett via samba-technical
On Tue, 2018-05-01 at 04:15 +0100, Olly Betts via samba-technical

Post by Olly Betts via samba-technical

Post by Andrew Bartlett via samba-technical
It would be better if it could either wrap or be wrapped by
cli_credentials_parse_file(). Ideally by adding
POPT_COMMON_CREDENTIALS to the popt table and reworking the code to use
it, just like client.c (for smbclient) does.

I did look at using POPT_COMMON_CREDENTIALS but there are short
option clashes with existing net options (-N and -S) and would
require doing something with the options in POPT_COMMON_CREDENTIALS
which net doesn't currently support.

-N we can probably work around (only used for groupmap). -S is harder,
perhaps removing that from the popt_common.c side would be best (and
fix source4/lib/cmdline/popt_credentials.c to match).
I'm hesitant about breaking scripts but making this use the common
credentials parsing code is really important (not just for parsing the
file, but for the rest of the things it gives us).

Some of the options which net doesn't currently support seem fairly
obvious to hook up, but what about --simple-bind-dn?

Post by Andrew Bartlett via samba-technical

Post by Olly Betts via samba-technical

Post by Andrew Bartlett via samba-technical
I realise this is a lot more work, but we need to unify this rather
than see further parser proliferation.

Given the concept's liked, I think a common parser shouldn't be too much
work (especially as I forked the existing one).

If you could give using the POPT_COMMON_CREDENTIALS one more try I
would appreciate it. There is a lot we gain if we unify this.

I'll take a look, though from what I've seen so far it might be hairier
than I can justify the time for.

I've already adjusted the code to reuse the existing parser (which is
actually simpler than the first patch), but haven't tested that yet.
I'll attach the revised patch so I can't lose it.

Cheers,
Olly

[Andrew Bartlett via samba-technical]

Post by Olly Betts via samba-technical

Post by Andrew Bartlett via samba-technical
On Tue, 2018-05-01 at 04:15 +0100, Olly Betts via samba-technical

Post by Olly Betts via samba-technical

Post by Andrew Bartlett via samba-technical
It would be better if it could either wrap or be wrapped by
cli_credentials_parse_file(). Ideally by adding
POPT_COMMON_CREDENTIALS to the popt table and reworking the code to use
it, just like client.c (for smbclient) does.

I did look at using POPT_COMMON_CREDENTIALS but there are short
option clashes with existing net options (-N and -S) and would
require doing something with the options in POPT_COMMON_CREDENTIALS
which net doesn't currently support.

-N we can probably work around (only used for groupmap). -S is harder,
perhaps removing that from the popt_common.c side would be best (and
fix source4/lib/cmdline/popt_credentials.c to match).
I'm hesitant about breaking scripts but making this use the common
credentials parsing code is really important (not just for parsing the
file, but for the rest of the things it gives us).

Some of the options which net doesn't currently support seem fairly
obvious to hook up, but what about --simple-bind-dn?

Just ignore it.

Post by Olly Betts via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Olly Betts via samba-technical

Post by Andrew Bartlett via samba-technical
I realise this is a lot more work, but we need to unify this rather
than see further parser proliferation.

Given the concept's liked, I think a common parser shouldn't be too much
work (especially as I forked the existing one).

If you could give using the POPT_COMMON_CREDENTIALS one more try I
would appreciate it. There is a lot we gain if we unify this.

I'll take a look, though from what I've seen so far it might be hairier
than I can justify the time for.

I agree, but I still feel this needs to be done right.

Post by Olly Betts via samba-technical
I've already adjusted the code to reuse the existing parser (which is
actually simpler than the first patch), but haven't tested that yet.
I'll attach the revised patch so I can't lose it.

Thanks. It will need an automated test.

Andrew Bartlett