Two diffs to add paramater self-checking
Andrew Bartlett
abartlet at samba.org
Fri Jul 12 18:03:05 GMT 2002
David Collier-Brown wrote:
>
> These are for param/loadparm.c and utils/testparm.c, respectively,
> to put in the self-checking that's been languishing...
My comments apply to HEAD, btw - which branch was the patch intended
for?
> After these go in, I'll start tracking smb.conf changes
> again, so I can support them, any you can delete
> samba-patches 298 (Self-check patches for 2.2.0 alpha 3,
> 11 Apr 2001)
>
> --dave
> --
> David Collier-Brown, | Always do right. This will gratify
> Performance & Engineering | some people and astonish the rest.
> Americas Customer Engineering, | -- Mark Twain
> (905) 415-2849 | davecb at canada.sun.com
>
> ------------------------------------------------------------------------
> --- loadparm.c 2002/07/11 00:46:17 1.1
> +++ loadparm.c 2002/07/11 23:54:35
> @@ -557,6 +557,9 @@
> static BOOL handle_vfs_object(char *pszParmValue, char **ptr);
> static BOOL handle_source_env(char *pszParmValue, char **ptr);
> static BOOL handle_netbios_name(char *pszParmValue, char **ptr);
> +static BOOL handle_netbios_aliases(char *pszParmValue,char **ptr);
> +static BOOL handle_password_server(char *pszParmValue, char **ptr);
> +static BOOL validate_netbios_name(char *pszParmValue);
> static BOOL handle_winbind_uid(char *pszParmValue, char **ptr);
> static BOOL handle_winbind_gid(char *pszParmValue, char **ptr);
> static BOOL handle_wins_server_list(char *pszParmValue, char **ptr);
> @@ -1813,6 +1816,7 @@
> service * pserviceDest);
> static void copy_service(service * pserviceDest,
> service * pserviceSource, BOOL *pcopymapDest);
> +static BOOL globals_ok(void);
> static BOOL service_ok(int iService);
> static BOOL do_parameter(char *pszParmName, char *pszParmValue);
> static BOOL do_section(char *pszSectionName);
> @@ -2182,52 +2186,245 @@
> }
> }
>
> +
> +
> /***************************************************************************
> -Check a service for consistency. Return False if the service is in any way
> -incomplete or faulty, else True.
> -***************************************************************************/
> -static BOOL service_ok(int iService)
> -{
> - BOOL bRetval;
> +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;
> + char *wins;
> + static BOOL filesChecked = False;
> +
> + /* Look for inconstancies in roughly decreasing order of severity, */
> +
> + /* Check for both wins server and wins support, make sure server wins. */
> + /* DCB: The data structure changes in 2.0.7, requiring this check. */
> + wins = Globals.szWINSserver;
> + if (Globals.bWINSsupport == True && wins != NULL
> + && ! (wins[0] == '\0' || strwicmp(wins, "127.0.0.1") == 0)) {
> + DEBUG(0,( "ERROR: both wins server = yes and wins support == yes, "
> + "using wins server \"%s\".\n", wins));
> + Globals.bWINSsupport = False;
> + }
Looks good.
> + /* Make sure we have a lock directory, but only once. */
> + if (filesChecked == False) {
> + if (!directory_exist(Globals.szLockDir, &st)) {
> + DEBUG(0,("ERROR: lock directory \"%s\" does not exist\n",
> + Globals.szLockDir));
> + bRetval = False;
> + }
> + else if ((st.st_mode & 0777) != 0755) {
> + DEBUG(0,("WARNING: lock directory \"%s\" should have "
> + "permissions 0755 for browsing and locks to work\n",
> + Globals.szLockDir));
> + }
> + filesChecked = True;
> + }
Good.
> + /* Security = (server or domain) requires password server to be set. */
> + if ((Globals.szPasswordServer == NULL || Globals.szPasswordServer[0] == '\0'
> +)
> + && (Globals.security == SEC_SERVER || Globals.security ==
> +SEC_DOMAIN)) {
> + DEBUG(0,("ERROR: security = server and security = domain "
> + "both require a password server.\n"));
> + bRetval = False;
> + }
No - when security = domain, it will try to find a DC if password server
is not set.
> + /* Password server should be a netbios name. */
> + if (Globals.szPasswordServer != NULL
> + && (strchr(Globals.szPasswordServer,'.') != NULL
> + || strlen(Globals.szPasswordServer) >= 15)) {
> + DEBUG(0,("ERROR: password server \"%s\" is not a legal "
> + "NetBIOS name, logons will fail.\n",
> + Globals.szPasswordServer));
> + bRetval = False;
> + }
No, we accept IP addresses too.
> + /* Check unix password sync prerequisites. */
> + if (Globals.bUnixPasswdSync) {
> + if (Globals.security != SEC_USER) {
> + DEBUG(0,("WARNING: unix password sync = yes requires "
> + "security = user.\n"));
> + }
No - we can have local accounts when secuirty=domain, and we should be
able to sync on those accounts.
> + if (Globals.bEncryptPasswords == False) {
> + DEBUG(0,("WARNING: unix password sync = yes requires "
> + "encrypt passwords = yes.\n"));
> + }
> + }
No - if encrypt passwords = no, then without this no password change at
all is done.
> + /* Be sure update encrypted is done with NON-encrypted passwords. */
> + if (Globals.bUpdateEncrypt && Globals.bEncryptPasswords == False) {
> + DEBUG(0,("WARNING: update encrypted = yes requires encrypt "
> + "passwords = no.\n"));
> + }
This one makes sense.
> - 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;
> - }
> + /* Er, should unix password sync be automatic if smb password file exists?
> +*/
>
> - /* 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 unix password sync, check the prerequisites.*/
> + if (Globals.bUnixPasswdSync) {
> + /* See if the program is executable. */
> + { pstring passwd_prog;
> + char *p;
> + pstrcpy(passwd_prog, Globals.szPasswdProgram);
> + if ((p = strchr(passwd_prog, ' ')) != NULL) {
> + *p = '\0';
> + }
> + if (access(passwd_prog, X_OK) == -1) {
> + DEBUG(0,("WARNING: the unix password sync program \"%s\" "
> + "can't be executed, errno was %d (\"%s\").\n",
> + Globals.szPasswdProgram,
> + errno, strerror(errno)));
> + }
> + }
>
> - 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());
> - }
> + /* And the passwd chat isn't expecting the old password. */
> + if (strstr(Globals.szPasswdChat, "%o") != 0) {
> + DEBUG(0,("WARNING: the 'passwd chat' script [%s] depends on "
> + "getting the old password via the %%o substitution. With "
> + "encrypted passwords this is not possible.\n",
> + Globals.szPasswdChat));
> + }
> +
> + }
Even with encrypted passwords, I understand that Win98 still sends the
old plaintext. But for every other OS, I understand this is correct.
> + /* Check for announcing as something other than NT, which can */
> + /* interefere with serving browse lists. */
> + if (Globals.announce_as != ANNOUNCE_AS_NT_SERVER) {
> + DEBUG(0,( "WARNING: announce as not set to \"NT\", this may "
> + "interfere with browsing.\n"));
> + }
> + return True;
> +}
>
> - /* 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);
> +/***************************************************************************
> +Check a service for consistency. Return False if the service is in any way
> +incomplete or faulty, else True.
> +***************************************************************************/
> +static BOOL service_ok(int iService)
> +{
> + BOOL bRetval = True;
> + struct stat buf;
> + service *s = ServicePtrs[iService];
> +
> + /* Look for inconstancies in roughly decreasing order of severity, */
> + /* Test for a missing service name. */
> + if (s->szService == NULL || s->szService[0] == '\0') {
> + DEBUG(0,( "The following message indicates an internal error:\n"));
> + DEBUGADD(0,( "ERROR: No service name in service entry.\n"));
> + bRetval = False;
> + }
> +
> + /* Test for path not pointing to a dir (other tests follow). */
> + if (s->szPath != NULL && s->szPath[0] != '\0') {
> + if (stat(s->szPath,&buf) == -1) {
> + DEBUG(0,("ERROR: can't stat path %s in service [%s].\n",
> + s->szPath, s->szService));
> + DEBUGADD(0,("errno = %d (%s).\n", errno, strerror(errno)));
> + }
> + else if ((buf.st_mode & S_IFDIR) != S_IFDIR) {
> + DEBUG(0,("ERROR: Path %s in service [%s] isn't a directory.\n",
> + s->szPath, s->szService));
> + }
> + }
Watch out that people use a pile of % macro substituions here - which
could make this a false alarm...
> + /* Contradictions in terms: */
> + /* 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(s->szService,PRINTERS_NAME) == 0) {
> + if (!s->bPrint_ok) {
> + DEBUG(0,( "ERROR: printer service [%s] MUST be printable!\n",
> + s->szService));
> + s->bPrint_ok = True;
> + }
> + }
> +
> + /* Things that are inherently dangerous: WARNINGs. */
> +
> + /* 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) {
> + 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) {
> + 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) {
> + 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) {
> + 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) {
> + 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) {
> + DEBUG(0,( "WARNING: force create mode forces hidden bit on "
> + "on all file in share [%s].\n",
> + s->szService));
> + }
> + /* Implausible overrides and missing prerequisites: WARNINGs. */
> + /* If a service is flagged unavailable, log the fact at level 0. */
> + if (!s->bAvailable) {
> + 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) {
> + DEBUG(0,( "NOTICE: Service [%s] is unbrowsable, but browse "
> + "lists are being served.\n", s->szService));
> + }
Isn't this a quite standard setting for the [homes] share?
> + /* If we're syncing always, we need strict sync on too. */
> + if (s->bSyncAlways == True && s->bStrictSync == False) {
> + DEBUG(0,("NOTICE: In service [%s], sync always = yes, which requires "
> + "strict sync = yes.\n", s->szService));
> + }
> +
> + /* We ned oplocks for level2 oplocks. */
> + if (s->bLevel2OpLocks == True && s->bOpLocks == False) {
> + 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) {
> + DEBUG(0,("NOTICE: No path in service [%s], using %s\n",
> + s->szService,tmpdir()));
> + string_set(&s->szPath,tmpdir());
> + }
> +
> + return (bRetval);
> }
>
> static struct file_lists
> @@ -2318,6 +2515,9 @@
> {
> pstring netbios_name;
>
> + if (validate_netbios_name(pszParmValue) == False)
> + return False;
> +
> pstrcpy(netbios_name, pszParmValue);
>
> standard_sub_basic(netbios_name);
> @@ -2338,6 +2538,109 @@
> }
>
> /***************************************************************************
> +handle_netbios_aliases -- validate and insert multiple netbios-name
> +parameters. It's a pstring global, with the DOS_STRING attribute.
> +***************************************************************************/
> +static BOOL handle_netbios_aliases(char *pszParmValue, char **parm_ptr) {
> + char *p;
> + pstring buf;
> +
> + *buf = '\0';
> + 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);
> + unix_to_dos(*(char **)parm_ptr);
> + strupper(*(char **)parm_ptr);
> + return True;
> +}
> +
> +/***************************************************************************
> +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 (Globals.security != SEC_SERVER && Globals.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 (Globals.security == SEC_DOMAIN && *pszParmValue == '*') {
> + pstrcpy(buf,pszParmValue);
> + }
> + 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);
> + unix_to_dos(*(char **)parm_ptr);
> + strupper(*(char **)parm_ptr);
> + return True;
> +}
> +
> +/**************************************************************************
> +validate a single netbios-name
> +**************************************************************************/
> +static char *legalNetbiosChars = "abcdefghijklmnopqrstuvwxyz"
> + "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_-";
> +
> +static BOOL validate_netbios_name(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));
> + *p = '\0';
> + DEBUGADD(0,("which is only legal in DNS domain names.\n"));
> + DEBUGADD(0,("Name has been truncated to \"%s\".\n",
> + pszParmValue));
> + }
> +
> + /* Alternative 1b: manage over-long name by truncating it */
> + len = strlen(pszParmValue);
> + if (len > 15) {
> + pszParmValue[15] = '\0';
> + len = 15;
> + DEBUG(0,("netbios name is longer than 15 characters, "
> + "and has been truncated to \"%s\".\n", pszParmValue));
> + }
> +
> + /* 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;
> +}
> +
> +
> +
> +/***************************************************************************
> Do the work of sourcing in environment variable/value pairs.
> ***************************************************************************/
>
> @@ -3495,6 +3798,8 @@
> string_set(&Globals.szWINSserver, "127.0.0.1");
>
> }
> + /* If we get here, check globals. */
> + bRetval = globals_ok();
>
> return (bRetval);
> }
>
> ------------------------------------------------------------------------
> --- testparm.c.old Fri Jul 12 14:07:35 2002
> +++ testparm.c Fri Jul 12 14:07:43 2002
> @@ -42,11 +42,133 @@
> extern FILE *dbf;
>
> /***********************************************
> -Here we formerly did a set of 'hard coded' checks for bad
> -configuration settings, called do_global_checks(void).
> -This is now done in loadparm, in globals_ok() -- Dave C-B
> + Here we do a set of 'hard coded' checks for bad
> + configuration settings.
> ************************************************/
Is this diff the right way around?
> +static int do_global_checks(void)
> +{
> + int ret = 0;
> + SMB_STRUCT_STAT st;
> +
> + if (lp_security() == SEC_DOMAIN && !lp_encrypted_passwords()) {
> + printf("ERROR: in 'security=domain' mode the 'encrypt passwords' parameter must also be set to 'true'.\n");
> + ret = 1;
> + }
Nitpicking, but this pointless configuration is actually valid... (and
yes, we should warn on it ;-).
> + if (lp_wins_support() && *lp_wins_server()) {
> + printf("ERROR: both 'wins support = true' and 'wins server = <server>' \
> +cannot be set in the smb.conf file. nmbd will abort with this setting.\n");
> + ret = 1;
> + }
Probably valid, but we might want to consider wins replication here.
Also, in HEAD tridge allows muliple wins server groups - where this
would be quite valid.
> + if (!directory_exist(lp_lockdir(), &st)) {
> + printf("ERROR: lock directory %s does not exist\n",
> + lp_lockdir());
> + ret = 1;
> + } else if ((st.st_mode & 0777) != 0755) {
> + printf("WARNING: lock directory %s should have permissions 0755 for browsing to work\n",
> + lp_lockdir());
> + ret = 1;
> + }
> +
> + if (!directory_exist(lp_piddir(), &st)) {
> + printf("ERROR: pid directory %s does not exist\n",
> + lp_piddir());
> + ret = 1;
> + }
> +
> + /*
> + * Password server sanity checks.
> + */
> +
> + if((lp_security() == SEC_SERVER || lp_security() == SEC_DOMAIN) && !lp_passwordserver()) {
> + pstring sec_setting;
> + if(lp_security() == SEC_SERVER)
> + pstrcpy(sec_setting, "server");
> + else if(lp_security() == SEC_DOMAIN)
> + pstrcpy(sec_setting, "domain");
> +
> + printf("ERROR: The setting 'security=%s' requires the 'password server' parameter be set \
> +to a valid password server.\n", sec_setting );
> + ret = 1;
> + }
> +
> + /*
> + * 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) {
> + printf("ERROR: the 'unix password sync' parameter is set and there is no valid 'passwd program' \
> +parameter.\n" );
> + ret = 1;
> + } 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) {
> + printf("ERROR: the 'unix password sync' parameter is set and the 'passwd program' (%s) \
> +cannot be executed (error was %s).\n", truncated_prog, strerror(errno) );
> + ret = 1;
> + }
> + }
> +
> +#ifdef WITH_PAM
> + }
> +#endif
> +
> + if(lp_passwd_chat() == NULL) {
> + printf("ERROR: the 'unix password sync' parameter is set and there is no valid 'passwd chat' \
> +parameter.\n");
> + ret = 1;
> + }
> +
> + /*
> + * Check that we have a valid script and that it hasn't
> + * been written to expect the old password.
> + */
> +
> + if(lp_encrypted_passwords()) {
> + if(strstr( lp_passwd_chat(), "%o")!=NULL) {
> + printf("ERROR: the 'passwd chat' script [%s] expects to use the old plaintext password \
> +via the %%o substitution. With encrypted passwords this is not possible.\n", lp_passwd_chat() );
> + ret = 1;
> + }
> + }
> + }
> +
> + if (lp_status(-1) && lp_max_smbd_processes()) {
> + printf("ERROR: the 'max smbd processes' parameter is set and the 'status' parameter is set to 'no'.\n");
> + ret = 1;
> + }
> +
> + if (strlen(lp_winbind_separator()) != 1) {
> + printf("ERROR: the 'winbind separator' parameter must be a single character.\n");
> + ret = 1;
> + }
> +
> + if (*lp_winbind_separator() == '+') {
> + printf("'winbind separator = +' might cause problems with group membership.\n");
> + }
> +
> + return ret;
> +}
>
> static void usage(char *pname)
> {
> @@ -124,6 +246,8 @@
> }
>
> printf("Loaded services file OK.\n");
> +
> + ret = do_global_checks();
>
> for (s=0;s<1000;s++) {
> if (VALID_SNUM(s))
--
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
More information about the samba-technical
mailing list