Bug 7342 - Don't ucase configured realm
Summary: Don't ucase configured realm
Status: RESOLVED WONTFIX
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: Config Files (show other bugs)
Version: unspecified
Hardware: All Other
: P3 normal
Target Milestone: ---
Assignee: Andrew Bartlett
QA Contact: Samba QA Contact
URL: http://marc.info/?t=126772172500006&r...
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-09 15:28 UTC by Benjamin Coddington
Modified: 2021-08-05 11:53 UTC (History)
3 users (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
no flags 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.
Comment 9 Andrew Bartlett 2019-03-14 17:28:59 UTC
(In reply to Volker Lendecke from comment #6)
In the meantime lp_realm() has become a 'constant' string parameter and no longer goes via lp_string().  The generation of the loadparm system makes this hard to find, but it is declared in docs-xml/smbdotconf/base/realm.xml via 'constant=1' and is visible in the generated bin/default/lib/param/param_functions.c.
Comment 10 Volker Lendecke 2021-08-05 11:28:57 UTC
(In reply to Andrew Bartlett from comment #9)
> (In reply to Volker Lendecke from comment #6)
> In the meantime lp_realm() has become a 'constant' string parameter and no
> longer goes via lp_string().  The generation of the loadparm system makes
> this hard to find, but it is declared in docs-xml/smbdotconf/base/realm.xml
> via 'constant=1' and is visible in the generated
> bin/default/lib/param/param_functions.c.

As far as I can see, the "realm" parameter is now handled via the handle_realm() function where the strupper() happens unconditionally.

Question to the authentication developers: Do we still need/want this parameter? If not, what shall we do with this bug report?
Comment 11 Andrew Bartlett 2021-08-05 11:42:14 UTC
This really feels like a local patch to me.  

So much in the rest of Samba has come to assume the force-uppercase that while your particular use case might work, things like the AD DC are more likely to become unhappy (I've not checked). 

As force-uppercase is well entrenched in the AD DC, I think testing this (and untested code is broken code) would be quite difficult.

Sorry!
Comment 12 Andrew Bartlett 2021-08-05 11:45:55 UTC
I would note that the thread at http://marc.info/?t=126772172500006&r=1&w=2 linked in the URL is quite insightful and useful context.
Comment 13 Volker Lendecke 2021-08-05 11:47:25 UTC
Andrew, handing this over to you. I think you have much more to say about all of this. Thanks for taking care!
Comment 14 Andrew Bartlett 2021-08-05 11:53:57 UTC
Benjamin,

I'm going to, regretfully, close this as WONTFIX as we can't afford to carry and test the additional complexity in our authentication code. 

Sorry,

Andrew Bartlett