Bug 10392 - TLS private key permissions
TLS private key permissions
Status: NEW
Product: Samba 4.1 and newer
Classification: Unclassified
Component: Other
4.1.4
All Linux
: P5 minor
: ---
Assigned To: Andrew Bartlett
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-01-21 20:44 UTC by Michael Brown
Modified: 2014-02-28 17:21 UTC (History)
1 user (show)

See Also:


Attachments
Proposed patch (1.54 KB, patch)
2014-01-22 03:33 UTC, Michael Brown
abartlet: review+
metze: review-
Details
rebased patch - no code change (1.54 KB, patch)
2014-01-30 20:36 UTC, Michael Brown
no flags Details
updated patch, added signed-off-by (1.59 KB, patch)
2014-01-30 20:38 UTC, Michael Brown
no flags Details
Patch for v4-1-test (1.70 KB, patch)
2014-02-02 17:38 UTC, Stefan Metzmacher
no flags Details
Patch for v4-0-test (1.70 KB, patch)
2014-02-02 17:39 UTC, Stefan Metzmacher
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Brown 2014-01-21 20:44:44 UTC
While setting up TLS:

[2014/01/16 12:19:42.945796,  0] ../source4/lib/tls/tls_tstream.c:1125(tstream_tls_params_server)
  Invalid permissions on TLS private key file '/var/lib/samba/private/tls/star.netdirect.ca-key.pem':
  owner uid 0 should be 0, mode 0400 should be 0600

0400 should *also* be a valid mode for the keyfile :)
Comment 1 Michael Brown 2014-01-22 03:33:38 UTC
Created attachment 9608 [details]
Proposed patch
Comment 2 Andrew Bartlett 2014-01-23 02:42:40 UTC
Comment on attachment 9608 [details]
Proposed patch

Reviewed-by: Andrew Bartlett <abartlet@samba.org>

Thanks.  If you don't get a second reviewer here shortly, please post it to samba-technical with the above maker I'll make sure it gets in.
Comment 3 Stefan Metzmacher 2014-01-30 20:29:58 UTC
Comment on attachment 9608 [details]
Proposed patch

This doesn't apply to current master.
Can you also add a Signed-off-by: line?
Comment 4 Michael Brown 2014-01-30 20:36:40 UTC
Created attachment 9622 [details]
rebased patch - no code change

rebased against master - no change
Comment 5 Michael Brown 2014-01-30 20:38:26 UTC
Created attachment 9623 [details]
updated patch, added signed-off-by

added Signed-off-by:
Comment 6 Stefan Metzmacher 2014-01-30 20:47:11 UTC
(In reply to comment #5)
> Created attachment 9623 [details]
> updated patch, added signed-off-by
> 
> added Signed-off-by:

Thanks!

You're messing up tabs in the attachments,
all tabs are whitespaces.

It only applies with git am --ignore-space-change,
then I replaces whitespaces with tabs again.

I've pushed this to autobuild.
Comment 7 Michael Brown 2014-01-30 22:37:09 UTC
(In reply to comment #6)
> You're messing up tabs in the attachments,
> all tabs are whitespaces.

Apologies! I was rushing it and forgot about that detail.

Bad terminal copy-paster.
Comment 8 Stefan Metzmacher 2014-02-02 17:38:54 UTC
Created attachment 9634 [details]
Patch for v4-1-test
Comment 9 Stefan Metzmacher 2014-02-02 17:39:18 UTC
Created attachment 9635 [details]
Patch for v4-0-test
Comment 10 Stefan Metzmacher 2014-02-04 18:52:54 UTC
Ok, this generates this now...

invalid permissions on file '/memdisk/metze/W/b138235/samba/bin/ab/promoted_dc/private/tls/key.pem': has 0600 should be 0400'.

I think we need a better way. Maybe file_check_permissions()
should get allow_perms and deny_perms. And we would call it
with allow_perms = 0400 and deny_perms = 0177. And bits in none
of them are ignored.
Comment 11 Andrew Bartlett 2014-02-28 04:43:27 UTC
(In reply to comment #10)
> Ok, this generates this now...
> 
> invalid permissions on file
> '/memdisk/metze/W/b138235/samba/bin/ab/promoted_dc/private/tls/key.pem': has
> 0600 should be 0400'.
> 
> I think we need a better way. Maybe file_check_permissions()
> should get allow_perms and deny_perms. And we would call it
> with allow_perms = 0400 and deny_perms = 0177. And bits in none
> of them are ignored.

I think we should revert the original patch for now, until such a patch is written, so we don't have this in 4.2.
Comment 12 Michael Brown 2014-02-28 17:21:17 UTC
(In reply to comment #11)
Yeah, it does generate that spurious warning… I'll rework it with allowed_mask and deny_mask.