Thank you for this PR.
The general organization looks good. I haven't fully reviewed the new function yet, however I see there are no comments.
Please ad a general comment before the new function that give a general overview about what is the intention of each function.

Whitin the function please add comments about what are the effects of the calls you make, as the cap library and operation is rather obscure to most people it is valuable to explain along what is going on and why it is being done.

A good comment is:
/ The next call shaves yak ABC because we need its fur for the winter /

A bad comment is:
/ shave_yak() shaves the yak /

Simo.

This error message doesn't match the check (i.e., you don't actually look for cap_set_proc)

Can you sort these while you're here?

Should this be a fatal error? Would like to hear an argument in either direction.

I'm also not convinced this should be fatal.

Thanks for the PR! Supplementing Simo's review with some comments inline.

This error message doesn't match the check (i.e., you don't actually look for cap_set_proc)

Fixed.

Can you sort these while you're here?

Sorted.

Should this be a fatal error? Would like to hear an argument in either direction.

ret = prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0) - without setting of SECBIT_KEEP_CAPS flag it's impossible to raise any of capability after a UID switch because all the cap sets are cleared.

I'm also not convinced this should be fatal.

ret = drop_caps() - because of SECBIT_KEEP_CAPS flag after user id switch the non-privileged user gains all the capabilities in the permitted set.

grep Cap /proc/"$(pgrep gssproxy)"/status
CapInh: 0000000000000000
CapPrm: 0000003fffffffff
CapEff: 0000000000000000
CapBnd: 0000003fffffffff
CapAmb: 0000000000000000

capsh --decode='0000003fffffffff' | tr ',' '\n'
0x0000003fffffffff=cap_chown
cap_dac_override
cap_dac_read_search
cap_fowner
cap_fsetid
cap_kill
cap_setgid
cap_setuid
cap_setpcap
cap_linux_immutable
cap_net_bind_service
cap_net_broadcast
cap_net_admin
cap_net_raw
cap_ipc_lock
cap_ipc_owner
cap_sys_module
cap_sys_rawio
cap_sys_chroot
cap_sys_ptrace
cap_sys_pacct
cap_sys_admin
cap_sys_boot
cap_sys_nice
cap_sys_resource
cap_sys_time
cap_sys_tty_config
cap_mknod
cap_lease
cap_audit_write
cap_audit_control
cap_setfcap
cap_mac_override
cap_mac_admin
cap_syslog
cap_wake_alarm
cap_block_suspend
cap_audit_read

This means that user can raise whatever he wants.

So, i guess both should be fatal as long as gssproxy supports access by the program name.
Another suggestion is using pre-caution about non-privileged user limitations ( this problem and for example

systemctl try-reload-or-restart gssproxy
journalctl -n 5 -u gssproxy -g "Keytab"
Keytab /etc/krb5.keytab has no content (-1765328203)

non-privileged user has no permission to read system keytab.
)

Of course, thank you all for review! 👍

Our style doesn't trail the closing */ - it should be on the line above.

Declaration at top of block, please.

I don't understand this comment, sorry!

I'd prefer goto done; cleanup for this - put the (checked) cap_free() after the done:. In general having fewer exit paths makes leakiness easier to reason about.

Same deal on this function.

Thanks @slev. Some more comments inline, but that should be it for me. Will wait to see what @simo says as well.

Our style doesn't trail the closing */ - it should be on the line above.

Done.

Declaration at top of block, please.

Done.

I don't understand this comment, sorry!

Rephrased.

I'd prefer goto done; cleanup for this - put the (checked) cap_free() after the done:. In general having fewer exit paths makes leakiness easier to reason about.

Label applied.

@rharwood , ready for next round :)
Thank you!

The code itself looks ok, however 2 points:
1) We should make cap support optional, seem easy as they are just 2 functions and a configure check
2) Caps should be manipulated only if needed (ie running as user), atm it looks to me this code is called also when gssproxy is running as root with all the needed capabilities, seems unnecessary.

1) I will check
2) if the key "run_as_user" is missing in config then no dropping privileges and no caps setting are executed:

int drop_privs(struct gp_config *cfg)                                           
{                                                                               
    char buf[2048];                                                             
    struct passwd *pw, pws;                                                     
    int ret;                                                                    
    if (cfg->proxy_user == NULL) {                                              
        /* not dropping privs */                                                
        return 0;                                                               
    }
    ...

One more thing. I see that there are no tests for "run_as_user" option. Is there an implementation plan for them?

Ah I forgot you check within the function ... can you make the check explicit before calling the function instead ?

If (cfg->proxy_user) {
    ret = drop_privs(cfg);
    ...
}

As for tests, I do not think we have optional tests, and run_As_user requires to urn as root, but our tests (IIRC) are all built to be run as non root ...

Ah I forgot you check within the function ... can you make the check explicit before calling the function instead ?
If (cfg->proxy_user) {
ret = drop_privs(cfg);
...
}

This code is already in the codebase (not related to my PR). But of course, I can change this.

As for tests, I do not think we have optional tests, and run_As_user requires to urn as root, but our tests (IIRC) are all built to be run as non root ...

Same here. I will try to get a look.

Yes please add a separate commit that moves the check into gssproxy.c so that it is clear from there that we do no privs dropping if run_as_user is not specified.

Please take a look.

Final question, I think. You make a change such that we don't go through the drop_privs() logic if we don't have run_as_user set; this is fine with the code in master right now. But isn't there some value in running through the drop_caps() part of that, even if we're still going to run as root? Also, don't we want PR_SET_KEEPCAPS set for the running as root case?

Also, please take a look at the CI failure.

@rharwood I requested the change to not go through drop_privs()

Ah I see. Looking at all the commits at once made this a bit confusing. So there's no actual change, just where the check is.

I think my question still stands though - do we want to run through drop_caps/PR_SET_KEEPCAPS when running as root? Because as written we don't.

(CI needs addressing as well.)

Ah I see. Looking at all the commits at once made this a bit confusing. So there's no actual change, just where the check is.
I think my question still stands though - do we want to run through drop_caps/PR_SET_KEEPCAPS when running as root? Because as written we don't.

We don't want to drop anything when running as root.
But if someone specifies 'run_as_user=root' then capabilities will be dropped.
I guess that it's not expected behavior.

(CI needs addressing as well.)

Fixed.
Should I drop dependency on libcap in the spec file?

On Wed, 2019-01-09 at 10:12 +0000, stanislav levin wrote:

We don't want to drop anything when running as root.
But if someone specifies 'run_as_user=root' then capabilities will be dropped.
I guess that it's not expected behavior.

I wonder if this is something we want to keep and document actually, so
that people can run as root and drop privs if they want ...

As I understand there are no required changes. Right?

On Wed, 2019-01-09 at 10:12 +0000, stanislav levin wrote:

We don't want to drop anything when running as root.
But if someone specifies 'run_as_user=root' then capabilities will be dropped.
I guess that it's not expected behavior.

I wonder if this is something we want to keep and document actually, so
that people can run as root and drop privs if they want ...

Wait, I'm confused again. Why don't we want to drop caps as root? And if we're considering an option to drop caps... maybe it should be the default?

As I understand there are no required changes. Right?

Not that I'm aware of. Just trying to understand (and agree) on the above. Please bear with us while we bikeshed for a moment :)

(Per IRC conversation, we don't want to drop caps as root because we rely on dac_override and the like.)

I'll merge presently.

Commit 2ca45253 fixes this pull-request