Description of problem: SELinux context is only displayed, you can't edit it. patch Additional info: I have a patch for review that adds editing capabilities to what is currently the SELinux context label. The URLs to get the src rpm/patch are: http://people.redhat.com/jantill/nautilus+selinux-src/ http://people.redhat.com/jantill/patches/nautilus-2.15.92.1-selinux.patch It "works for me"(tm), now the bad news: . Not sure I'm allowed to steal the c mnemonic, we really need a mnemonic though. . MLS/MCS isn't tested by me yet, and the UI for that sucks the big one (it really needs check boxes for MCS and two seperated combo boxes for MLS). You could argue that about the entire UI, but this was the best I could come up with and it's usable (IMO). . Set context fail just resets the entry to the previous value, this sucks. . Completion changes focus if the pointer is where the completion list appears, this then emits a leave-focus event ... which is then tries to apply the "new context", which fails and resets the entry box. . We don't add the matchpathcon() type to the completion list (values in selinux_customizable_types_path()). . Added by hand user_home_t and tmp_t to the completion list ... this almost certainly means there's more (adding the matchpathcon() type might make this go away). . Completion doesn't help you if you want to change the entire label, but we can probably mark that as unsupported for the short term. (and there are workarounds). . some of the code could/should be cleaned up, but you could say that about a lot of the preferences dialog C file :O. . Probably other stuff I've forgotten about.
Created attachment 134890 [details] Initial patch for nautilus SELinux editing Putting the first beta version here.
Upstream builds with -Werror from cvs, and this has warnings: nautilus-file.c: In function 'nautilus_file_set_selinux_context': nautilus-file.c:3647: warning: passing argument 1 of 'selinux_trans_to_raw_context' discards qualifiers from pointer target type fm-properties-window.c: In function 'attach_selinux_comp_edit_field': fm-properties-window.c:1728: warning: assignment discards qualifiers from pointer target type Furthermore, this is just an entry where you can modify a strange string that you need a phd in selinux to understand. There is no way to know what you're supposed/allowed to write there, nor is there a simple way to get back to an old safe version if you accidentally changed it. In this release we make the normal unix settings easier to understand (unless you activate the advanced mode), and people seem to like that much better than the old version that exposed the full unix lowlevel settings. However, we're already getting requests to be able to remove the selinux label, because its just scaring people. Just making it an entry isn't an option. Isn't there some way you can make this easier to use? For instance, have a dropdown with nice translated natural language strings describing various common contextes. Possibly with a "custom" element that pops up a dialog where you can write whatever selinux context you want.
Having a drop down available, with a small list of options (when in not-advanced mode) ... isn't that hard to do, now. It'd only alter the type, but that'll be all 99% of people want anyway (MCS checkboxes might be useful later, as I said). It'll be highly specific and statically define a bunch of targeted policy information, but it can say things like "Allowed to run as CGI, without file IO" and "Allowed to run as CGI, and can read files". But I wanted to get the "can do anything" UI working first. I'm surprised about the warnings, as I fixed a few warnings in fm-properties-window.c, I've fixed those two.
Ok, this contains the "simple" version. As I said above, the "explanations" are all hand coded (there's no where external to get them from atm.) ... and they could do with Dan/etc. checking them. It might be useful to have these as tooltips or something for completion to, but I'm not sure how/if I can do that. However it does a weird thing when you select the popup (it has a large blank space), which I assume is something obvious that I'm not doing ... but I can't see it. Still doesn't include a matchpathcon() like type, and MCS (both of which we probably need for GA).
Created attachment 134967 [details] Simple and advanced type setting Uploading the new patch, also have src and binary rpms at my people page.
Created attachment 134987 [details] SELinux nautilus patch, including matchpathcon etc. This is ready to go into rawhide/FC6-betaX now, IMO. You can change everything in advanced mode, and can change to any of the customizable types in simple mode (and there are convertions for a significant number of types into english). Both have sorted lists, with pixmaps for current and default types (STOCK_OK and STOCK_HOME). Packages from: http://people.redhat.com/jantill/nautilus+selinux-src/ http://people.redhat.com/jantill/nautilus+selinux-FC6/
First of all, there are lots of indentation and code style-guide issue. Upstream (== me) is pretty picky about this, and I want to be able to just commit this patch upstream later. The nautilus styleguide is in docs/style-guide.html. Examples of things that need to be fixed: +#ifdef HAVE_SELINUX + security_context_t rcontext = NULL; +#endif No initializations at declaration. +static void maybe_gtk_entry_set_text(GtkEntry *entry, const char *val) +{ No space before "(". Type and function name on separate rows +static void attach_selinux_data_edit_field(GtkEntry *entry, + char *attr_value, + char *def_attr_value); + Declarations on top of file. (and no space before "(") + // fm_report_error_setting_selinux (file, result, NULL); No c++ comments + if (!gtk_combo_box_get_active_iter (comb, &iter)) + return; + else Always use brackets + else + { Brackets on same line as if/else + + return (NULL); No paranthesis around return value. +#ifdef HAVE_SELINUX +# include <selinux/selinux.h> +#endif Includes at top of file Real comments: There is no support for selinux context setting in the recursive permissions setting. I'm not sure about the icons. First of all, the icon should be to the left of the text. That way its not indented to fit all text lengths. Secondly, i'm not sure what the OK icon is supposed to mean. When experimenting it seems to only show what value you last changed it to. And it overrides the home icon for the users home dir. Furthermore i don't think OK is a good icon to use here. GTK_STOCK_APPLY might be better, but as i said, i'm not sure what its supposed to mean. + HACK_TYPE("cupsd_etc_t", "CUPS printer configuration."); These strings are not translated, they need _("") markup around the actual string. gettext works on the source, and doesn't expand macros. Also, the translations won't be in FC6, because this code is not upstream, so the gnome tranlators won't see it. Can't we put all these strings things in a file somewhere (with translations) that nautilus reads? That way selinux can ship with and update these strings. The strings shouldn't end with a period. That looks silly in the UI. + HACK_TYPE("samba_share_t", "Shared via. CIFS (samba)."); This has a stray dot in it. + if (!nautilus_file_is_local (file)) + return (NULL); + +#ifdef HAVE_SELINUX + uri = nautilus_file_get_uri (file); + fname = gnome_vfs_get_local_path_from_uri (uri); nautilus_file_is_local doesn't do what you think it does. It will return FALSE for a mounted NFS filesystem. Just look at the value returned from gnome_vfs_get_local_path_from_uri and check for NULL. - GtkWidget *icon_chooser; + gpointer icon_chooser; /* passing & to g_object_add_weak_pointer */ I don't think its actually nicer to save it as a gpointer even if we avoid a cast. Its better to have the real type info in the data structures so you know what type of object it is when reading the code. +static void selinux_split_cntx (char *cntx, + const char **ret_attr_u, + const char **ret_attr_r, + const char **ret_attr_t, + const char **ret_attr_s) This modifies cntx. Would be nice with a comment on that. + fname_ctypes = selinux_customizable_types_path (); + /* read types, one per line... */ + if ((ioc_ctypes = g_io_channel_new_file (fname_ctypes, "r", &errc))) + { + char *data = NULL; + + while ((data = cust_type_next_line (ioc_ctypes))) + cust_types = g_slist_prepend (cust_types, data); + + g_io_channel_unref (ioc_ctypes); + } Seem easier to use g_file_get_contents & g_strsplit than this. + /* FIXME: this should be cached, but threading makes it hurt */ + for (scan = cust_types; scan; scan = scan->next) + g_free (scan->data); Threading? Nautilus doesn't use threads.
Created attachment 135096 [details] Style and minor bug fixes, still not recursive This contains almost all of the above, note that icon_chooser now produces a warning about aliasing again (which is what you want). I also haven't converted to using g_file_get_contents(), although there is caching of the customizable types now. It still doesn't change the label recursively, it'll be another day or two to get that working I think. One problem is that it's not obvious how to tweak the UI so that you can apply permission changes recursively without affecting the security context. Also, with regard to the icons. The Home icon represents the default security context for that file (Ie. what install would label it as) ... the _OK icon shows what label it had when the preferences dialog was opened. I tried to choose icons that made some sense, but...
Bringing up a properties dialog with multiple files with different selinux context selected looks weird. I get the home icon on a line that says "--" in the dropdown, and the context displayed doesn't seem right. The other permissions widgets have a third "inconsistent" state for this meaning "use the current value for each file". The other combos use "---" for this. We also track the original setting for each file so that changing the value back to "---" restores the old values. The inconsistent state is also what is used for the recursive permission handling. If a folder is selected (so recursive permissions changing is availible) you can manually set a combo in an inconsistent state to not change something when applying recursively. Something similar could be done with the selinux stuff. + gtk_cell_layout_pack_start (GTK_CELL_LAYOUT (comb), cell, TRUE); + gtk_cell_layout_set_attributes (GTK_CELL_LAYOUT (comb), cell, + "stock-id", 2, NULL); + cell = gtk_cell_renderer_text_new (); + gtk_cell_layout_pack_start (GTK_CELL_LAYOUT (comb), cell, TRUE); Pass expand = FALSE in the pack_start calls to match the rest of the dialog. Some further style issues: + char *translated = NULL; + char *raw = NULL; + char *uri = NULL; + char *fname = NULL; + security_context_t rcontext = NULL; + set_permissions_callback(handle, result, new_info, callback_data); +static void maybe_gtk_entry_set_text(GtkEntry *entry, const char *val) + char *old_val = NULL; +static void selinux_done_editing(FMPropertiesWindow *window, + char *selinux_context) + } + else { fm-properties-window.c: In function 'attach_selinux_data_edit_field': fm-properties-window.c:1839: warning: unused variable 'fname_ctypes'
Created attachment 136586 [details] Does recursive, multiple nodes (with the same context) Ok, I think this is basically it. It might be nice to be able to change from inconsistent label's when you have multiple contexts and changing MLS/MCS/etc. when in simple mode isn't possible, but those can be other features for other days. This should do everything, and follow the nautilus rules. Can you take a look and add it to FC6, if there are no major problems?
The recursive change is a bit weird. Whenever you change the permissions recursively on a directory you will make all the files in the directory have the same context as the directory, as there is no way to say "don't change context" by setting it to inconsistent. Also, i fixed a bug in: + if (def_attr_val) { + selinux_split_cntx (def_attr_val, &dattr_u, &dattr_r, + &dattr_t, &dattr_s); + } Where dattr_t etc can be read uninitialized.
This is in nautilus-2.16.0-4.fc6.
Confirmed it has been fixed in nautilus-2.16.0-5.el5