Two patches to add self-checks, part 1 of 7
Andrew Bartlett
abartlet at samba.org
Wed Apr 23 15:03:49 GMT 2003
On Sat, 2003-04-12 at 09:04, Dave Collier-Brown wrote:
> I haven't died, I just got additional responsibilities
> at work...
And I've just been a bit busy - there are just a few things we need to
tweak...
> The following is a reprise of the self-check code
> I did for 2.x, retargeted to head/3.x. To get it
> past the automated sentinal, I'll send it in a series of
> individual messages, the first two containing
> patches, then the patched files, then the Makefile
> to test the changes... .as there's nothing worse than
> a test that hasn't been tested (;-))
I almost applied it, but I'm not sure about some of the assertions being
made. In particular, I think some of the configurations that are
restricted actually make sense:
> /***************************************************************************
> + Check globals for consistency. Return False if something fatal will happen.
> + The messages use ERROR, WARNING and NOTICE the same way service_ok does.
> + This replaces do_global_checks in utils/testparm.c, and should be called at
> + least once, and possibly each time the smb.conf file is reread.
> + ***************************************************************************/
> + static BOOL globals_ok(void)
> + {
> + BOOL bRetval = True;
> + SMB_STRUCT_STAT st;
> + static BOOL filesChecked = False;
> +
> + /* Look for inconstancies in roughly decreasing order of severity, */
> +
> + /* Make sure we have a lock directory, but only once. */
> + if (filesChecked == False) {
> + /* Check lock directory. Self-test 1. */
> + if (!directory_exist(lp_lockdir(), &st)) {
> + DEBUG(0,("ERROR: lock directory \"%s\" does not exist\n",
> + lp_lockdir()));
> + bRetval = False;
> + }
> + /* Self-test 2. */
> + else if ((st.st_mode & 0777) != 0755) {
> + DEBUG(0,("WARNING: lock directory \"%s\" should have "
> + "permissions 0755 for browsing and locks to work\n",
> + lp_lockdir()));
> + }
> + if (!directory_exist(lp_piddir(), &st)) {
> + /* Self-test 3. */
> + DEBUG(0,("ERROR: pid directory %s does not exist\n",
> + lp_piddir()));
> + bRetval = False;
> + }
> + else if ((st.st_mode & 0777) != 0755) {
> + /* Self-test 4. */
> + DEBUG(0,("WARNING: pid directory %s should have "
> + "permissions 0755 for browsing to work\n",
> + lp_piddir()));
> + }
> + filesChecked = True;
> + }
> +
> + if (lp_security() == SEC_DOMAIN && !lp_encrypted_passwords()) {
> + /* Self-test 5. */
> + DEBUG(0,("ERROR: in 'security=domain' mode the 'encrypt passwords' "
> + "parameter must always be set to 'true'.\n"));
> + bRetval = False;
> + }
> +
> +#ifdef undef
> + if (lp_wins_support() && lp_wins_server_list()) {
> + /* This is actually wrong: Andrew Bartlett noticed that */
> + /* one could have oneself in the list. I'm unsure of */
> + /* how to write a good test for that. --davecb */
> + DEBUG(0,("ERROR: 'wins support = true' and 'wins server = "
> + "<server list>' cannot both be true.\n"));
> + bRetval = False;
> + }
> +#endif /* undef */
> +
> + /* Self-test 6. */
> + if (*lp_hosts_equiv() && !lp_hostname_lookups()) {
> + DEBUG(0,("ERROR: The setting 'hosts equiv = %s' requires that "
> + "'hostname lookups = yes'.\n", lp_hosts_equiv()));
> + bRetval = False;
> + }
> +
> + /* Password chat sanity checks. */
> + if (lp_security() == SEC_USER && lp_unix_password_sync()) {
> + /* Check that we have a valid lp_passwd_program() if not using pam. */
> +#ifdef WITH_PAM
> + if (!lp_pam_password_change()) {
> +#endif
> + if (lp_passwd_program() == NULL) {
> + /* Self-test 7. */
> + DEBUG(0,("ERROR: the 'unix password sync' parameter "
> + "is set but there is no valid 'passwd "
> + "program' parameter.\n"));
> + bRetval = False;
> + }
> + else {
> + pstring passwd_prog;
> + pstring truncated_prog;
> + char *p;
> +
> + pstrcpy( passwd_prog, lp_passwd_program());
> + p = passwd_prog;
> + *truncated_prog = '\0';
> + next_token(&p, truncated_prog, NULL, sizeof(pstring));
> +
> + if (access(truncated_prog, F_OK) == -1) {
> + /* Self-test 7 redux. */
> + DEBUG(0,("ERROR: the 'unix password sync' parameter "
> + "is set and the 'passwd program' (%s) "
> + "cannot be executed (error was %s).\n",
> + truncated_prog, strerror(errno)));
> + bRetval = False;
> + }
> + }
> +
> +#ifdef WITH_PAM
> + }
> +#endif
> + }
> + if (strlen(lp_winbind_separator()) != 1) {
> + /* Self-test 8. */
> + DEBUG(0,("ERROR: the 'winbind separator' parameter must "
> + "be a single character.\n"));
> + bRetval = False;
> + }
> + if (*lp_winbind_separator() == '+') {
> + /* Self-test 9 */
> + DEBUG(0,("WARNING: winbind separator = + may cause "
> + "problems with group membership.\n"));
> + }
We need to be able to 'warn' without filling logs. Perhaps make level 1
or 2 for warnings? Or some way that testparm can log these at level 0,
but others at 1/2?
> +
> + /* Password server should be a netbios name or IP address. */
> + if (lp_passwordserver != NULL) {
> + if (strchr(lp_passwordserver(),'.') != NULL &&
> + !isdigit(*lp_passwordserver())) {
> + /* Self-test 10. */
> + DEBUG(0,("ERROR: password server \"%s\" is not a legal "
> + "NetBIOS name or IP address, logons will fail.\n",
> + lp_passwordserver()));
> + bRetval = False;
DO logons really fail? To my mind, these would work fine, as long as
the host we are pointing too is in DNS. Given we are moving to AD, this
is quite a reasonable possibility.
> + }
> + else if (strlen(lp_passwordserver()) >= 15) {
> + /* Self-test 11. */
> + DEBUG(0,("ERROR: password server \"%s\" is too long to "
> + "be a legal NetBIOS name, logons will fail.\n",
> + lp_passwordserver()));
> + bRetval = False;
> + }
> + }
> + /* Be sure update encrypted is done with NON-encrypted passwords. */
> + if (lp_update_encrypted() && lp_encrypted_passwords()) {
> + /* Self-test 12. */
> + DEBUG(0,("WARNING: update encrypted = yes requires encrypt "
> + "passwords = yes.\n"));
Requires 'encrypt passwords = no'.
> + }
> + /* Er, should unix password sync be automatic if a smb */
> + /* password file exists? */
> + /* If unix password sync, check the prerequisites.*/
> + if (lp_unix_password_sync()) {
> + /* See if the program is executable. Self-test 13.*/
> + { pstring passwd_prog;
> + char *p;
> + pstrcpy(passwd_prog, lp_passwd_program());
> + if ((p = strchr(passwd_prog, ' ')) != NULL) {
> + *p = '\0';
> + }
> + if (access(passwd_prog, X_OK) == -1) {
> + /* Self-test 13. */
> + DEBUG(0,("WARNING: the unix password sync program \"%s\" "
> + "can't be executed, errno was %d (\"%s\").\n",
> + lp_passwd_program(),
> + errno, strerror(errno)));
> + }
> + }
> + /* There is a password chat value to use. */
> + if (lp_passwd_chat() == NULL || *lp_passwd_chat() == NULL) {
> + /* Self-test 14. */
> + DEBUG(0,("ERROR: the 'unix password sync' parameter is set "
> + "but there is no valid 'passwd chat' parameter.\n"));
> + bRetval = False;
> + }
> +
> + /* And the passwd chat isn't expecting the old password. */
> + if (strstr(lp_passwd_chat(), "%o") != 0) {
> + /* Self-test 15. */
> + DEBUG(0,("WARNING: the 'passwd chat' script [%s] depends on "
> + "getting the old password via the %%o substitution. With "
> + "encrypted passwords and NT clients this will not work.\n",
> + lp_passwd_chat()));
> + }
> +
> + }
> +
> + /* Check for announcing as something other than NT, which can */
> + /* interefere with serving browse lists. */
> + if (lp_announce_as() != ANNOUNCE_AS_NT_SERVER) {
> + /* Self-test 16. */
> + DEBUG(0,( "WARNING: announce as not set to \"NT\", this may "
> + "interfere with browsing.\n"));
> + }
If we are going to level-0 warn on every use of this option, why is it
an option at all? (and yes, I would like to kill off options).
> + /* The following tests existed previously, they just moved here. */
> + if (lp_algorithmic_rid_base() < BASE_RID) {
> + /* Try to prevent admin foot-shooting, we can't put */
> + /* algorithmic rids below 1000, that's the 'well known RIDs' on NT */
> + /* Self-test 17. */
> + DEBUG(0,( "ERROR: The 'algorithmic rid base' mustbe equal to or above %lu\n", BASE_RID));
> + }
> + if (lp_algorithmic_rid_base() & 1) {
> + /* Self-test 17b. */
> + DEBUG(0,( "ERROR: The 'algorithmic rid base' must be even.\n"));
> + }
> + /* Self-tests 18-19 reserved for future globals_ok tests. */
These are good.
> + return True;
> + }
> +
> +/***************************************************************************
> Check a service for consistency. Return False if the service is in any way
> incomplete or faulty, else True.
> ***************************************************************************/
> @@ -2263,41 +2470,113 @@ incomplete or faulty, else True.
> static BOOL service_ok(int iService)
> {
> BOOL bRetval;
> + struct stat buf;
> + service *s = ServicePtrs[iService];
> +
> + /* Look for inconstancies in roughly decreasing order of severity, */
> + /* Things that are inherently dangerous: WARNINGs. */
> +
> + /* Test for path not pointing to a dir. */
> + /* This has to be macro-expanded for stat, as noted by Andrew Bartlett. */
> + if (s->szPath != NULL && s->szPath[0] != '\0') {
> + if (stat(lp_pathname(iService),&buf) == -1) {
> + /* Self-test 20. */
> + DEBUG(0,("WARNING: can't stat path \"%s\" in service [%s].\n",
> + lp_pathname(iService), s->szService));
> + DEBUGADD(0,("errno = %d (%s).\n", errno, strerror(errno)));
> + }
> + else if ((buf.st_mode & S_IFDIR) != S_IFDIR) {
> + /* Self-test 21. */
> + DEBUG(0,("WARNING: Path %s in service [%s] isn't a directory.\n",
> + lp_pathname(iService), s->szService));
> + }
> + }
> +
> + /* Look for map archive forced to fail by bad create mask. */
> + /* Ditto map system, map hidden. */
> + if (s->bMap_archive == True && (s->iCreate_mask & 0100) == 0) {
> + /* Self-test 22. */
> + DEBUG(0,( "WARNING: map archive = yes in share [%s], but create "
> + "mask doesn't allow setting archive bit (100 octal).\n",
> + s->szService));
> + }
> +
> + if (s->bMap_system == True && (s->iCreate_mask & 0010) == 0) {
> + /* Self-test 22b. */
> + DEBUG(0,( "WARNING: map system = yes in share [%s], but create "
> + "mask doesn't allow setting system bit (010 octal).\n",
> + s->szService));
> + }
> +
> + if (s->bMap_hidden == True && (s->iCreate_mask & 0001) == 0) {
> + /* Self-test 22c. */
> + DEBUG(0,( "WARNING: map hidden = yes in share [%s], but create "
> + "mask doesn't allow setting hidden bit (001 octal).\n",
> + s->szService));
> + }
> +
> + /* And see if force create mode sets any of the same three bits */
> + if ((s->iCreate_force_mode & 0100) != 0) {
> + /* Self-test 23. */
> + DEBUG(0,( "WARNING: force create mode forces archive bit on "
> + "on all files in share [%s].\n",
> + s->szService));
> + }
> + if ((s->iCreate_force_mode & 0010) != 0) {
> + /* Self-test 23b. */
> + DEBUG(0,( "WARNING: force create mode forces system bit on "
> + "on all files in share [%s].\n",
> + s->szService));
> + }
> + if ((s->iCreate_force_mode & 0001) != 0) {
> + /* Self-test 23c. */
> + DEBUG(0,( "WARNING: force create mode forces hidden bit on "
> + "on all file in share [%s].\n",
> + s->szService));
> + }
> + /* Implausible overrides and missing prerequisites: NOTICESs. */
> + /* If a service is flagged unavailable, log the fact at level 0. */
> + if (!s->bAvailable) {
> + /* Self-test 24. */
> + DEBUG(1,( "NOTICE: Service [%s] is marked available = no.\n",
> + s->szService));
> + }
> +
> + /* If it's unbrowsable but we're serving browse lists, log that too. */
> + if (s->bBrowseable == False && Globals.bBrowseList == True
> + && strwicmp(s->szService,HOMES_NAME) != 0) {
> + /* Self-test 25. */
> + DEBUG(0,( "NOTICE: Service [%s] is unbrowsable, but browse "
> + "lists are being served.\n", s->szService));
> + }
I'm not quite sure on this one - isn't the idea here just to hide a
couple of shares from the normally visible list?
> + /* If we're syncing always, we need strict sync on too. */
> + if (s->bSyncAlways == True && s->bStrictSync == False) {
> + /* Self-test 26. */
> + DEBUG(0,("NOTICE: In service [%s], sync always = yes, which requires "
> + "strict sync = yes.\n", s->szService));
> + }
> +
> + /* We need oplocks for level2 oplocks. */
> + if (s->bLevel2OpLocks == True && s->bOpLocks == False) {
> + /* Self-test 27. */
> + DEBUG(0,("NOTICE: In service [%s], level 2 oplocks = yes, which "
> + "requires oplocks = yes as well.\n", s->szService));
> + }
> +
> + /* Depending on defaults for proper execution: NOTICEs.*/
> + /* Test for missing path, substitute the default if unset. */
> + if (s->szPath != NULL && s->szPath[0] == '\0'
> + && strwicmp(s->szService,HOMES_NAME) != 0) {
> + /* Self-test 24. */
> + DEBUG(0,("NOTICE: No path in service [%s], using %s\n",
> + s->szService,tmpdir()));
> + string_set(&s->szPath,tmpdir());
> + }
> +
> + return (bRetval);
> + }
>
> - bRetval = True;
> - if (ServicePtrs[iService]->szService[0] == '\0') {
> - DEBUG(0, ("The following message indicates an internal error:\n"));
> - DEBUG(0, ("No service name in service entry.\n"));
> - bRetval = False;
> - }
> -
> - /* The [printers] entry MUST be printable. I'm all for flexibility, but */
> - /* I can't see why you'd want a non-printable printer service... */
> - if (strwicmp(ServicePtrs[iService]->szService, PRINTERS_NAME) == 0) {
> - if (!ServicePtrs[iService]->bPrint_ok) {
> - DEBUG(0, ("WARNING: [%s] service MUST be printable!\n",
> - ServicePtrs[iService]->szService));
> - ServicePtrs[iService]->bPrint_ok = True;
> - }
> - /* [printers] service must also be non-browsable. */
> - if (ServicePtrs[iService]->bBrowseable)
> - ServicePtrs[iService]->bBrowseable = False;
> - }
> -
> - if (ServicePtrs[iService]->szPath[0] == '\0' &&
> - strwicmp(ServicePtrs[iService]->szService, HOMES_NAME) != 0) {
> - DEBUG(0, ("No path in service %s - using %s\n",
> - ServicePtrs[iService]->szService, tmpdir()));
> - string_set(&ServicePtrs[iService]->szPath, tmpdir());
> - }
> -
> - /* If a service is flagged unavailable, log the fact at level 0. */
> - if (!ServicePtrs[iService]->bAvailable)
> - DEBUG(1, ("NOTE: Service %s is flagged unavailable.\n",
> - ServicePtrs[iService]->szService));
> -
> - return (bRetval);
> -}
>
> static struct file_lists {
> struct file_lists *next;
> @@ -2391,11 +2670,11 @@ static BOOL handle_netbios_name(const ch
> BOOL ret;
> pstring netbios_name;
>
> - pstrcpy(netbios_name, pszParmValue);
>
> + if (validate_netbios_name(pszParmValue) == False)
> + return False;
> + pstrcpy(netbios_name, pszParmValue);
> standard_sub_basic(current_user_info.smb_name, netbios_name,sizeof(netbios_name));
> -
> -
> ret = set_global_myname(netbios_name);
> string_set(&Globals.szNetbiosName,global_myname());
>
> @@ -2405,6 +2684,95 @@ static BOOL handle_netbios_name(const ch
> return ret;
> }
>
> +
> +
> + /***************************************************************************
> + handle_password_server -- validate and insert multiple netbios-name
> + parameters. It's a pstring global.
> + ***************************************************************************/
> + static BOOL handle_password_server(char *pszParmValue, char **parm_ptr) {
> + char *p;
> + pstring buf;
> +
> + *buf = '\0';
> + if (lp_security() != SEC_SERVER && lp_security() != SEC_DOMAIN) {
> + DEBUG(0,("WARNING: password server set to \"%s\", ",pszParmValue));
> + DEBUGADD(0,("but security is neither server nor domain.\n"
> + "password server value ignored\n"));
> + return True;
> + }
> +
> + /* A "*" by itself means search for Primary or Backup Domain controllers */
> + if (lp_security() == SEC_DOMAIN && *pszParmValue == '*') {
> + pstrcpy(buf,pszParmValue);
This is valid (if stupid - even easier to spoof) for security=server
too.
> + }
> + else {
> + for (p=strtok(pszParmValue, " \t"); p != NULL; p=strtok(NULL," \t")) {
> + if (validate_netbios_name(p) == False)
> + return False;
> + pstrcat(buf,p);
> + pstrcat(buf," ");
> + }
> + buf[MIN(strlen(buf)-1,sizeof(buf))] = '\0';
> + }
> +
> + /* I've treated it here as an uppercase pstring. P_USTRING --davecb */
> + string_set(parm_ptr,buf);
> + strupper(*(char **)parm_ptr);
> + return True;
> + }
> +
> +/**************************************************************************
> +Validate a single netbios-name, and render it legal if possible.
> +under control of #define FIX
> +**************************************************************************/
> +static char *legalNetbiosChars = "abcdefghijklmnopqrstuvwxyz"
> + "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-";
> +
> +/* #define FIX */
> +static BOOL validate_netbios_name(const char *pszParmValue) {
> + int len, i;
> + char *p;
> +
> + /* Alternative 1a: manage FQDN by removing domain */
> + if ((p = strchr(pszParmValue, '.')) != NULL) {
> + DEBUG(0,("WARNING: netbios name \"%s\" contained a dot,\n",
> + pszParmValue));
> +#ifdef FIX
> + *p = '\0';
> + DEBUGADD(0,("which is only legal in DNS domain names.\n"));
> + DEBUGADD(0,("Name has been truncated to \"%s\".\n",
> + pszParmValue));
> +#else
> + return False;
> +#endif
> + }
> +
> + /* Alternative 1b: manage over-long name by truncating it */
> + len = strlen(pszParmValue);
> + if (len > 15) {
> +#ifdef FIX
> + pszParmValue[15] = '\0';
> + len = 15;
> + DEBUG(0,("netbios name is longer than 15 characters, "
> + "and has been truncated to \"%s\".\n", pszParmValue));
> +#else
> + DEBUG(0,("netbios name is longer than 15 characters\n", pszParmValue));
> + return False;
> +#endif
> + }
> +
> + /* Alternative 2: fail non-alphanumerics. */
> + if ((i = strspn(pszParmValue,legalNetbiosChars)) < len) {
> + DEBUG(0,("netbios name \"%s\" contains non-alphanumeric character "
> + "\"%c\", and cannot be set.\n", pszParmValue,
> + pszParmValue[i]));
> + return False;
> + }
> + return True;
> + }
> +
> +
> static BOOL handle_workgroup(const char *pszParmValue, char **ptr)
> {
> BOOL ret;
> @@ -3697,6 +4065,9 @@ BOOL lp_load(const char *pszFname, BOOL
> }
>
> init_iconv();
> +
> + /* If we get here, check globals. */
> + bRetval = globals_ok();
>
> return (bRetval);
> }
--
Andrew Bartlett abartlet at pcug.org.au
Manager, Authentication Subsystems, Samba Team abartlet at samba.org
Student Network Administrator, Hawker College abartlet at hawkerc.net
http://samba.org http://build.samba.org http://hawkerc.net
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.samba.org/archive/samba-technical/attachments/20030424/66211da7/attachment.bin
More information about the samba-technical
mailing list