Bug 7342 - Don't ucase configured realm
Don't ucase configured realm
Status: NEW
Product: Samba 3.6
Classification: Unclassified
Component: Config Files
unspecified
All Other
: P3 normal
: ---
Assigned To: Volker Lendecke
Samba QA Contact
http://marc.info/?t=126772172500006&r...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-09 15:28 UTC by Benjamin Coddington
Modified: 2011-10-31 21:24 UTC (History)
1 user (show)

See Also:


Attachments
Here's a fix to preserve realm case (2.29 KB, patch)
2010-04-09 15:28 UTC, Benjamin Coddington
no flags Details
Second try (3.46 KB, patch)
2010-04-13 15:48 UTC, Benjamin Coddington
vl: review-
Details
using lp_string (3.48 KB, patch)
2010-04-16 13:51 UTC, Benjamin Coddington
kseeger: review? (vl)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Coddington 2010-04-09 15:28:13 UTC
We are one of the few that need to preserve case in kerberos realm names.  The following seems to make sense - administrators ought to use the proper case when configuring a realm name.


diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
index 579f847..f5fd985 100644
--- a/source3/param/loadparm.c
+++ b/source3/param/loadparm.c
@@ -147,6 +147,8 @@ struct global {
 	char *szPasswordServer;
 	char *szSocketOptions;
 	char *szRealm;
+	char *szEffectiveRealm;
+	bool *bRealmPreserveCase;
 	char *szAfsUsernameMap;
 	int iAfsTokenLifetime;
 	char *szLogNtTokenCommand;
@@ -995,13 +997,22 @@ static struct parm_struct parm_table[] = {
 #ifdef WITH_ADS
 	{
 		.label		= "realm",
-		.type		= P_USTRING,
+		.type		= P_STRING,
 		.p_class	= P_GLOBAL,
-		.ptr		= &Globals.szRealm,
-		.special	= NULL,
+		.ptr		= &Globals.szEffectiveRealm,
+		.special	= handle_realm,
 		.enum_list	= NULL,
 		.flags		= FLAG_BASIC | FLAG_ADVANCED | FLAG_WIZARD,
 	},
+	{
+		.label		= "realm preserve case",
+		.type		= P_BOOL,
+		.p_class	= P_GLOBAL,
+		.ptr		= &Globals.bRealmPreserveCase,
+		.special	= handle_realm_preserve_case,
+		.enum_list	= NULL,
+		.flags		= FLAG_BASIC | FLAG_ADVANCED,
+	},
 #endif
 	{
 		.label		= "netbios name",
@@ -5351,7 +5362,7 @@ FN_GLOBAL_STRING(lp_passwd_program, &Globals.szPasswdProgram)
 FN_GLOBAL_STRING(lp_passwd_chat, &Globals.szPasswdChat)
 FN_GLOBAL_STRING(lp_passwordserver, &Globals.szPasswordServer)
 FN_GLOBAL_STRING(lp_name_resolve_order, &Globals.szNameResolveOrder)
-FN_GLOBAL_STRING(lp_realm, &Globals.szRealm)
+FN_GLOBAL_STRING(lp_realm, &Globals.szEffectiveRealm)
 FN_GLOBAL_CONST_STRING(lp_afs_username_map, &Globals.szAfsUsernameMap)
 FN_GLOBAL_INTEGER(lp_afs_token_lifetime, &Globals.iAfsTokenLifetime)
 FN_GLOBAL_STRING(lp_log_nt_token_command, &Globals.szLogNtTokenCommand)
@@ -7228,6 +7239,25 @@ static bool handle_workgroup(int snum, const char *pszParmValue, char **ptr)
 	return ret;
 }
 
+static bool handle_realm(int snum, const char *pszParmValue, char **ptr)
+{
+	string_set(ptr, pszParmValue);
+	string_set(&Globals.szRealm, pszParmValue);
+	if (!Globals.bRealmPreserveCase)
+		strupper_m(*ptr);
+
+	return True;
+}
+
+static bool handle_realm_preserve_case(int snum, const char *pszParmValue, char **ptr)
+{
+	string_set(&Globals.szEffectiveRealm, Globals.szRealm);
+	if (!(*(bool *)ptr = lp_bool(pszParmValue)))
+		strupper_m(Globals.szEffectiveRealm);
+
+	return True;
+}
+
 static bool handle_netbios_scope(int snum, const char *pszParmValue, char **ptr)
 {
 	bool ret;
Comment 1 Benjamin Coddington 2010-04-09 15:28:58 UTC
Created attachment 5619 [details]
Here's a fix to preserve realm case
Comment 2 Volker Lendecke 2010-04-12 03:59:14 UTC
Why don't you replace lp_realm() with a call that looks at "realm preserve case", doing the strupper on demand?

Seems simpler to me.

Volker
Comment 3 Benjamin Coddington 2010-04-12 08:31:24 UTC
You'd rather strupper a value every time you need it than do it once?  It looks like the other lp_ variables point to memory, not a function, so that's the way I did it.  It's also close to the way samba4 uses two pointers for szRealm_lower and szRealm_upper.  Or, are you suggesting that you'd replace every call to lp_realm?
Comment 4 Volker Lendecke 2010-04-12 08:45:24 UTC
I would suggest to take away the

FN_GLOBAL_STRING(lp_realm, &Globals.szRealm)

definition an replace it with a function of the prototype 

char *lp_realm(void) {
    /* new code */
}

that does the new magic. If you are concerned about the performance, you might also introduce a new parameter

"case insensitive realm"

that takes precedence over the

"realm"

parameter when set. "case insensitive realm" would then be a P_STRING instead of a P_USTRING. Your definition of the lp_realm function would then look if "case insensitive realm" is defined. If so, then it would return that value. If it is not defined (the default), then it would return the value of the "realm" parameter.

BTW, please also add a smb.conf manual page entry of any new parameter you choose. You might want to look at docs-xml/smbdotconf for templates.

Thanks,

Volker
Comment 5 Benjamin Coddington 2010-04-13 15:48:44 UTC
Created attachment 5632 [details]
Second try

Here's another shot at it.  Thanks for the guidance.
Comment 6 Volker Lendecke 2010-04-16 02:51:56 UTC
Comment on attachment 5632 [details]
Second try

This patch is the right approach, but it misses an important point: The existing definition of lp_realm goes through the function lp_string, which the new patch drops. lp_string does the %-Macro substitutions. It might be arguable if % macro substitutions are required for the realm parameter, but for compatibility reasons I would argue that this is necessary. If you want this patch in this form, you might want to start the pulic discussion about this issue on samba-technical@samba.org. Maybe I am just wrong with my 100% compatibility argument in this case.
Comment 7 Benjamin Coddington 2010-04-16 13:51:55 UTC
Created attachment 5635 [details]
using lp_string

Thanks for looking this over, that was an oversight on my part.  Here's the same approach including lp_string.
Comment 8 Karolin Seeger 2011-08-02 18:44:14 UTC
Re-assigning to Volker for patch review.