NSS has some tests for checking that certificates are correctly signed, but PSM does not have any, we should add some as we start to move to over chain verification options.

Comment on attachment 778038 [details] [diff] [review] test cert signature verification on chain building Review of attachment 778038 [details] [diff] [review]: ----------------------------------------------------------------- Overall, this is definitely a good thing to have. My main issues are with inconsistent whitespace around operators ("=", "+", ",", etc.) Other comments: Do you not need to add the generated cert files to a Makefile.in or moz.build file? ::: security/manager/ssl/tests/unit/test_cert_signatures.js @@ +13,5 @@ > + > +do_get_profile(); // must be called before getting nsIX509CertDB > +const certdb = Cc["@mozilla.org/security/x509certdb;1"].getService(Ci.nsIX509CertDB); > + > +var dict = new Object; pretty sure this line is unnecessary @@ +18,5 @@ > + > +var ca_usage1= 'SSL CA'; > +var ca_usage2= 'Client,Server,Sign,Encrypt,SSL CA,Status Responder'; > +var ee_usage1= 'Client,Server,Sign,Encrypt'; > +var ee_usage2= 'Client,Server,Sign,Encrypt,Status Responder'; I'm not seeing ee_usage2 used anywhere in the test... @@ +32,5 @@ > + 'ee-rsa-perturbed-int': "", > + 'p384-perturbed-int': "", > + 'ee-p384-perturbed-int': "", > + > + 'rsa-perturbed-ee': ca_usage2, Wait - what? Is the correct? Should a tampered ee cert be able to do all of this? @@ +43,5 @@ > +function load_ca(ca_name) { > + var ca_filename = ca_name + ".der"; > + addCertFromFile(certdb,"test_cert_signatures/" + ca_filename,'CTu,CTu,CTu'); > + > + do_print("ca_name=" + ca_name); I don't think we want to keep these do_prints for the final, for-checkin version. ::: security/manager/ssl/tests/unit/test_cert_signatures/generate.py @@ +13,5 @@ > +CA_min_ku = "keyUsage=critical, keyCertSign\n" > +CA_bad_ku = "keyUsage=digitalSignature, nonRepudiation, keyEncipherment, dataEncipherment, keyAgreement, cRLSign\n" > +EE_full_ku ="keyUsage=digitalSignature, nonRepudiation, keyEncipherment, dataEncipherment, keyAgreement, keyCertSign, cRLSign\n" > + > +Server_eku= "extendedKeyUsage=critical,serverAuth,clientAuth\n " Not every '=' has consistent whitespace around it - I'd go through and make sure they're all ' = '. @@ +16,5 @@ > + > +Server_eku= "extendedKeyUsage=critical,serverAuth,clientAuth\n " > + > + > +def generate_cert(key_type, name,ext_text,signer_key_filename = "",signer_cert_filename=""): Same kind of thing - not every ',' has a space after it @@ +19,5 @@ > + > +def generate_cert(key_type, name,ext_text,signer_key_filename = "",signer_cert_filename=""): > + global serial_num > + > + key_name = db + "/"+ name + ".key" whitespace here again - ' + ' @@ +39,5 @@ > + > + cert_name = srcdir + "/"+ name + ".der" > + if not signer_key_filename: > + signer_key_filename = key_name; > + os.system ("openssl x509 -req -sha256 -days 3650 -in "+csr_name+" -signkey "+signer_key_filename+" -set_serial "+str(serial_num)+" -extfile "+extensions_filename+" -outform DER -out "+cert_name) I found it useful in the OCSP stapling tests to echo out what system commands the generate script was issuing, so I could re-run them by hand and see what was going wrong. @@ +51,5 @@ > + [int_key, int_cert] = generate_cert(key_type, name,int_ext_text,ca_key,ca_cert) > + generate_cert(key_type, "ee-"+name,ee_ext_text,int_key,int_cert) > + return int_key, int_cert > + > +def perturbate_cert(cert_name): maybe just "perturb_cert" @@ +53,5 @@ > + return int_key, int_cert > + > +def perturbate_cert(cert_name): > + f = open(cert_name, 'r+b') > + f.seek(-3,2) #third byes from the end "# third byte from the end" @@ +55,5 @@ > +def perturbate_cert(cert_name): > + f = open(cert_name, 'r+b') > + f.seek(-3,2) #third byes from the end > + b = bytearray(f.read(1)) > + for i in range(len(b)): Do you really need to use a loop here? Isn't it just one byte?

Comment on attachment 778636 [details] [diff] [review] test cert signature verification on chain building (v2) Review of attachment 778636 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/test_cert_signatures.js @@ +1,2 @@ > +"use strict"; > +/* To regenerate the certificates for this test: Please document the purpose of this test. The purpose of this test is to verify that we correctly detect bad signatures on tampered certificates. But, we should also be verifying that the error we return is the correct error. Also, we should be testing that we honor the MD5 preference. @@ +72,5 @@ > + } > + > + // Now do the checks > + for (var key in dict) { > + var cert_name = key; for (var cert_name in dict) { @@ +76,5 @@ > + var cert_name = key; > + do_print("cert_name=" + cert_name); > + > + var cert; > + cert = certdb.findCertByNickname(null, cert_name); combine these lines together. @@ +81,5 @@ > + > + var verified = {}; > + var usages = {}; > + cert.getUsagesString(true, verified, usages); > + do_print("usages.value=" + usages.value); the do_print is not necessary because do_check_eq will print the value. ::: security/manager/ssl/tests/unit/test_cert_signatures/generate.py @@ +17,5 @@ > +Server_eku= "extendedKeyUsage=critical,serverAuth,clientAuth\n " > + > +pk_name = {"rsa": 'rsa', 'dsa': 'dsa', 'p384': 'secp384r1'} > + > +def generate_cert(key_type, name, ext_text, signer_key_filename = "", signer_cert_filename = ""): Rather than copy/paste/modify this from the existing code, please modify the existing code so that it can support the additional functionality. @@ +50,5 @@ > + > +def generate_int_and_ee(key_type, ca_key, ca_cert, name, int_ext_text, ee_ext_text): > + [int_key, int_cert] = generate_cert(key_type, name, int_ext_text, ca_key, ca_cert) > + generate_cert(key_type, "ee-" + name, ee_ext_text, int_key, int_cert) > + return int_key, int_cert Same here. This function seems like it already exists from an earlier patch. @@ +54,5 @@ > + return int_key, int_cert > + > +def perturbate_cert(cert_name): > + f = open(cert_name, 'r+b') > + f.seek(-3, 2) #third byes from the end Why did you choose the third byte from the end? Also, the typos in the comment make it unclear exactly what you mean. Also, why are we doing this as part of the generation step? We could just do the modification in the JS code as part of the test. And, in fact, we shouldn't even need to generate any new certificates for this test. Instead, we can just take some of the known-good certificates that have already been generated for other tests and perturb them. Then we wouldn't even need this script or any of these generated certs at all.

(In reply to Brian Smith (:briansmith), was bsmith@mozilla.com (:bsmith) from comment #5) > Comment on attachment 778636 [details] [diff] [review] > test cert signature verification on chain building (v2) > > Review of attachment 778636 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: security/manager/ssl/tests/unit/test_cert_signatures.js > @@ +1,2 @@ > > +"use strict"; > > +/* To regenerate the certificates for this test: > > Please document the purpose of this test. > > The purpose of this test is to verify that we correctly detect bad > signatures on tampered certificates. > > But, we should also be verifying that the error we return is the correct > error. > > Also, we should be testing that we honor the MD5 preference. Should we? md5 has been broken for signatures for a while. > > @@ +72,5 @@ > > + } > > + > > + // Now do the checks > > + for (var key in dict) { > > + var cert_name = key; > > for (var cert_name in dict) { > > @@ +76,5 @@ > > + var cert_name = key; > > + do_print("cert_name=" + cert_name); > > + > > + var cert; > > + cert = certdb.findCertByNickname(null, cert_name); > > combine these lines together. > > @@ +81,5 @@ > > + > > + var verified = {}; > > + var usages = {}; > > + cert.getUsagesString(true, verified, usages); > > + do_print("usages.value=" + usages.value); > > the do_print is not necessary because do_check_eq will print the value. > > ::: security/manager/ssl/tests/unit/test_cert_signatures/generate.py > @@ +17,5 @@ > > +Server_eku= "extendedKeyUsage=critical,serverAuth,clientAuth\n " > > + > > +pk_name = {"rsa": 'rsa', 'dsa': 'dsa', 'p384': 'secp384r1'} > > + > > +def generate_cert(key_type, name, ext_text, signer_key_filename = "", signer_cert_filename = ""): > > Rather than copy/paste/modify this from the existing code, please modify the > existing code so that it can support the additional functionality. > > @@ +50,5 @@ > > + > > +def generate_int_and_ee(key_type, ca_key, ca_cert, name, int_ext_text, ee_ext_text): > > + [int_key, int_cert] = generate_cert(key_type, name, int_ext_text, ca_key, ca_cert) > > + generate_cert(key_type, "ee-" + name, ee_ext_text, int_key, int_cert) > > + return int_key, int_cert > > Same here. This function seems like it already exists from an earlier patch. > > @@ +54,5 @@ > > + return int_key, int_cert > > + > > +def perturbate_cert(cert_name): > > + f = open(cert_name, 'r+b') > > + f.seek(-3, 2) #third byes from the end > > Why did you choose the third byte from the end? The signature of an x509 cert is always the last structure of the der encoding. I since I only want the signaure to be invalid I could perturb the from the end up to x bytes of the signature or decode the ans1 der struct. Also, the typos in the > comment make it unclear exactly what you mean. > > Also, why are we doing this as part of the generation step? because it was a good way to verify that the certs where actually broken. > > We could just do the modification in the JS code as part of the test. And, > in fact, we shouldn't even need to generate any new certificates for this > test. Instead, we can just take some of the known-good certificates that > have already been generated for other tests and perturb them. > > Then we wouldn't even need this script or any of these generated certs at > all. There are no DSA or EC keys in our testing store. Further I would prefer to have them separate as a way to have the test independence (storage is cheap). All the rest I agree

Comment on attachment 782846 [details] [diff] [review] test cert signature verification on chain building (v3) Review of attachment 782846 [details] [diff] [review]: ----------------------------------------------------------------- Basically, I have a bunch of nits. I think it would be good to have another look with those fixed, though, so r- for now. ::: security/manager/ssl/tests/unit/psm_common_py/CertUtils.py @@ +1,1 @@ > +import os,random MPL boilerplate? (for all of these files except the binary ones, actually) @@ +3,5 @@ > +def init_dsa(db_dir): > + dsa_key_params = db_dir + "/dsa_param.pem" > + os.system ("openssl dsaparam -out "+ dsa_key_params + " 2048") > + > +def generate_cert_generic(db_dir, dest_dir, serial_num, key_type, name, ext_text, signer_key_filename = "", signer_cert_filename = ""): In general for this file, some comments would be great. What does each function do, what are the arguments expected to be, and what preconditions must there be? @@ +15,5 @@ > + else: > + #assume is ec > + os.system("openssl ecparam -out "+ key_name+ " -name "+ key_type+ " -genkey"); > + csr_name = db_dir + "/"+ name + ".csr" > + os.system ("openssl req -new -key "+ key_name + " -days 3650 -extensions v3_ca -batch -out " + csr_name + " -utf8 -subj '/CN=" + name + "'") Should the validity period of the certs be a parameter? @@ +28,5 @@ > + signer_key_filename = key_name; > + os.system ("openssl x509 -req -sha256 -days 3650 -in " + csr_name + " -signkey " + signer_key_filename + " -set_serial "+ str(serial_num) + " -extfile " + extensions_filename + " -outform DER -out "+ cert_name) > + else: > + os.system ("openssl x509 -req -sha256 -days 3650 -in " + csr_name + " -CAkey " + signer_key_filename + " -CA " + signer_cert_filename + " -CAform DER -set_serial " + str(serial_num) + " -out " + cert_name + " -outform DER -extfile " + extensions_filename) > + #serial_num = serial_num + 1 looks like this line isn't necessary @@ +37,5 @@ > +def generate_int_and_ee_generic(db_dir, dest_dir, key_type, ca_key, ca_cert, name, int_ext_text, ee_ext_text): > + [int_key, int_cert] = generate_cert_generic(db_dir,dest_dir,random.randint(100,40000000),key_type, name, int_ext_text, ca_key, ca_cert) > + generate_cert_generic(db_dir, dest_dir, random.randint(100,40000000), key_type, "ee-" + name, ee_ext_text, int_key, int_cert) > + > + return int_key, int_cert why does this just return the intermediate key and cert? @@ +40,5 @@ > + > + return int_key, int_cert > + > + > +def generate_int_and_ee(db_dir, dest_dir, ca_key, ca_cert, name, int_ext_text, ee_ext_text): I'm not sure I see the usefulness of this function - maybe just use default values in your function declarations? ::: security/manager/ssl/tests/unit/test_cert_signatures.js @@ +1,5 @@ > +"use strict"; > +/* > + * The purpose of this test is to verify that we correctly detect bad signatures on tampered certificates. > + * Evenstually, we should also be verifying that the error we return is the correct error. > + * trailing space @@ +21,5 @@ > +var ca_usage1 = 'SSL CA'; > +var ca_usage2 = 'Client,Server,Sign,Encrypt,SSL CA,Status Responder'; > +var ee_usage1 = 'Client,Server,Sign,Encrypt'; > + > +var dict = { maybe "usagesMap" or something similar would be a more helpful name for this @@ +50,5 @@ > +function load_ca(ca_name) { > + var ca_filename = ca_name + ".der"; > + addCertFromFile(certdb, "test_cert_signatures/" + ca_filename, 'CTu,CTu,CTu'); > + > + do_print("ca_name=" + ca_name); I'm not sure any of the do_prints are necessary @@ +65,5 @@ > + // Load the ca into mem > + load_ca("ca-rsa"); > + load_ca("ca-p384"); > + load_ca("ca-dsa"); > + trailing space @@ +81,5 @@ > + > + var verified = {}; > + var usages = {}; > + cert.getUsagesString(true, verified, usages); > + do_print("usages.value=" + usages.value); like Brian said, this do_print shouldn't be necessary, particularly with the do_check_eq later ::: security/manager/ssl/tests/unit/test_cert_signatures/generate.py @@ +18,5 @@ > +CA_min_ku = "keyUsage=critical, keyCertSign\n" > +CA_bad_ku = "keyUsage=digitalSignature, nonRepudiation, keyEncipherment, dataEncipherment, keyAgreement, cRLSign\n" > +EE_full_ku = "keyUsage=digitalSignature, nonRepudiation, keyEncipherment, dataEncipherment, keyAgreement, keyCertSign, cRLSign\n" > + > +Server_eku= "extendedKeyUsage=critical,serverAuth,clientAuth\n " looks like there's a trailing space in the string here - is it necessary? @@ +20,5 @@ > +EE_full_ku = "keyUsage=digitalSignature, nonRepudiation, keyEncipherment, dataEncipherment, keyAgreement, keyCertSign, cRLSign\n" > + > +Server_eku= "extendedKeyUsage=critical,serverAuth,clientAuth\n " > + > +pk_name = {"rsa": 'rsa', 'dsa': 'dsa', 'p384': 'secp384r1'} the quoting style is inconsistent here

Comment on attachment 782846 [details] [diff] [review] test cert signature verification on chain building (v3) Review of attachment 782846 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/psm_common_py/CertUtils.py @@ +15,5 @@ > + else: > + #assume is ec > + os.system("openssl ecparam -out "+ key_name+ " -name "+ key_type+ " -genkey"); > + csr_name = db_dir + "/"+ name + ".csr" > + os.system ("openssl req -new -key "+ key_name + " -days 3650 -extensions v3_ca -batch -out " + csr_name + " -utf8 -subj '/CN=" + name + "'") Dont think so. Every 10 years we can regenerate the certs. @@ +37,5 @@ > +def generate_int_and_ee_generic(db_dir, dest_dir, key_type, ca_key, ca_cert, name, int_ext_text, ee_ext_text): > + [int_key, int_cert] = generate_cert_generic(db_dir,dest_dir,random.randint(100,40000000),key_type, name, int_ext_text, ca_key, ca_cert) > + generate_cert_generic(db_dir, dest_dir, random.randint(100,40000000), key_type, "ee-" + name, ee_ext_text, int_key, int_cert) > + > + return int_key, int_cert The ee-location is implied and the ee-key is not really neaded for enthing else @@ +40,5 @@ > + > + return int_key, int_cert > + > + > +def generate_int_and_ee(db_dir, dest_dir, ca_key, ca_cert, name, int_ext_text, ee_ext_text): will remove. ::: security/manager/ssl/tests/unit/test_cert_signatures.js @@ +50,5 @@ > +function load_ca(ca_name) { > + var ca_filename = ca_name + ".der"; > + addCertFromFile(certdb, "test_cert_signatures/" + ca_filename, 'CTu,CTu,CTu'); > + > + do_print("ca_name=" + ca_name); These are for debugging. (they do not appear in normal runs) it is good to know what cert is makign things fail (but I can remove this if you think is cleaner)

Comment on attachment 784447 [details] [diff] [review] test cert signature verification on chain building (v4.1) Review of attachment 784447 [details] [diff] [review]: ----------------------------------------------------------------- Awesome!

Comment on attachment 784447 [details] [diff] [review] test cert signature verification on chain building (v4.1) Review of attachment 784447 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/test_cert_signatures/generate.py @@ +25,5 @@ > + > + > +def perturbate_cert(cert_name): > + f = open(cert_name, 'r+b') > + f.seek(-3, 2) #third byte from the end to ensure we only touch the signature value Please document more clearly why you chose the third byte from the end instead of one of the bytes of tbsCertificate. The reason is that you don't want the verification to fail for a non-signature reason due to setting a certificate field to a non-default value.

Unfortunatelly we need to skip the tests on android. Here is a try run (that success on android) https://tbpl.mozilla.org/?tree=Try&rev=b2c9778d728c

Comment on attachment 796746 [details] [diff] [review] test cert signature verification on chain building (v4.4) Review of attachment 796746 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/psm_common_py/CertUtils.py @@ +48,5 @@ > + cert_name -- the filename of the output certificate (DER format) > + """ > + key_name = db_dir + "/"+ name + ".key" > + if key_type == 'rsa': > + os.system ("openssl genpkey -algorithm RSA -out "+ key_name + " -pkeyopt rsa_keygen_bits:2048") openssl genpkey and openssl gendsa isn't available in the openssl that ships with MozillaBuild.

(In reply to Brian Smith (:briansmith, was :bsmith@mozilla.com) from comment #18) > openssl genpkey and openssl gendsa isn't available in the openssl that ships > with MozillaBuild. '-days 3650'. So we are safe for 10 years. A comment you need to install openssl (version?) that supports it is enough IMO.

Comment on attachment 796746 [details] [diff] [review] test cert signature verification on chain building (v4.4) Review of attachment 796746 [details] [diff] [review]: ----------------------------------------------------------------- r- since the test is hard to track and 'decode'. Otherwise the test seems to be well written. I understand we create a batch of certs for each signing algo we support or want to test. But what is the content of the batch is hard to track. The names are not fully descriptive. I presume "*-perturbed-int-valid-ee.der" are broken CAs? "ee-*-perturbed-int-valid-ee" is a valid end entity cert signed by invalid CA? "*-valid-int-perturbed-ee.der" is an invalid end entity signed by a valid intermediate? Does it make sense to have also case perturbed-ca-perturbed-ee ? Next time please run only xpcshell-tests on try ;) ::: security/manager/ssl/tests/unit/test_cert_signatures.js @@ +1,4 @@ > +"use strict"; > +/* > + * The purpose of this test is to verify that we correctly detect bad signatures on tampered certificates. > + * Evenstually, we should also be verifying that the error we return is the correct error. Eventually @@ +19,5 @@ > +const certdb = Cc["@mozilla.org/security/x509certdb;1"].getService(Ci.nsIX509CertDB); > + > +var ca_usage1 = 'SSL CA'; > +var ca_usage2 = 'Client,Server,Sign,Encrypt,SSL CA,Status Responder'; > +var ee_usage1 = 'Client,Server,Sign,Encrypt'; it would be better to put these directly to the code. this is hard to track. maybe just better descriptive names might help? those will be long names probably :)) @@ +42,5 @@ > + 'ee-rsa-valid-int-perturbed-ee': "", > + 'p384-valid-int-perturbed-ee': ca_usage2, > + 'ee-p384-valid-int-perturbed-ee': "", > + 'dsa-valid-int-perturbed-ee': ca_usage2, > + 'ee-dsa-valid-int-perturbed-ee': "", nice, but check it is correct is not easy :) @@ +81,5 @@ > + > + var verified = {}; > + var usages = {}; > + cert.getUsagesString(true, verified, usages); > + var value = cert2usage[cert_name]; isn't there some python construct of |for| as in php is with => ? ::: security/manager/ssl/tests/unit/test_cert_signatures/generate.py @@ +59,5 @@ > + ca_cert, > + name + "-valid", > + CA_basic_constraints, > + ee_ext_text, > + key_type) Please add a big and clear comment what all this method produces. It's almost impossible to easily track what then the perturbate_cert method affects and do.

This is to address meyhemer's comments. See https://bugzilla.mozilla.org/show_bug.cgi?id=895601#c22. In short: better documentation of the python script and the mapping of certs2usages. Of his comments I could not find a better way to name the certs, so if you have ideas I welcome them,

Comment on attachment 819203 [details] [diff] [review] test cert signature verification on chain building (v5) Review of attachment 819203 [details] [diff] [review]: ----------------------------------------------------------------- Main concerns: - long lines (try to wrap at 80 characters) - generating more intermediates than we need to (see comment below) - maybe "tamper/tampered" is a better word than "perturb/perturbed" in this context ::: security/manager/ssl/tests/unit/psm_common_py/CertUtils.py @@ +19,5 @@ > + dsa_key_params = db_dir + "/dsa_param.pem" > + os.system ("openssl dsaparam -out "+ dsa_key_params + " 2048") > + > + > +def generate_cert_generic(db_dir, dest_dir, serial_num, key_type, name, ext_text, signer_key_filename = "", signer_cert_filename = ""): There are a lot of long lines in this file - can we wrap them at 80 characters? ::: security/manager/ssl/tests/unit/test_cert_signatures.js @@ +1,1 @@ > +"use strict"; MPL boilerplate? @@ +39,5 @@ > + 'ee-p384-perturbed-int-valid-ee': "", > + 'dsa-perturbed-int-valid-ee': "", > + 'ee-dsa-perturbed-int-valid-ee': "", > + > + 'rsa-valid-int-perturbed-ee': int_usage, If I understand correctly, this is testing an RSA intermediate that should be good (it just happens that there's an end-entity certificate around that has been tampered with). However, we already test a good RSA intermediate ("rsa-valid"), right? So why do we need to generate and test another? Or am I misunderstanding? (same with each of dsa and p384) ::: security/manager/ssl/tests/unit/test_cert_signatures/generate.py @@ +1,1 @@ > +#!/usr/bin/python MPL boilerplate?

Comment on attachment 820476 [details] [diff] [review] test cert signature verification on chain building (v6.1) Review of attachment 820476 [details] [diff] [review]: ----------------------------------------------------------------- Awesome - just some tiny nits. r=me with those. ::: security/manager/ssl/tests/unit/psm_common_py/CertUtils.py @@ +62,5 @@ > + #assume is ec > + os.system("openssl ecparam -out "+ key_name+ " -name "+ key_type + > + " -genkey"); > + csr_name = db_dir + "/"+ name + ".csr" > + os.system ("openssl req -new -key "+ key_name + " -days 3650" + nit: some of the '+'s don't have leading spaces, it looks like @@ +125,5 @@ > + > + """ > + [int_key, int_cert] = generate_cert_generic(db_dir, dest_dir, > + random.randint(100,40000000), > + key_type, "int-" + name, nit: trailing space ::: security/manager/ssl/tests/unit/test_cert_signatures.js @@ +5,5 @@ > + > +"use strict"; > +/* > + * The purpose of this test is to verify that we correctly detect bad > + * signatures on tampered certificates. Eventually, we should also be nit: extra space before "we" here ::: security/manager/ssl/tests/unit/test_cert_signatures/generate.py @@ +32,5 @@ > + > +def tamper_cert(cert_name): > + f = open(cert_name, 'r+b') > + f.seek(-3, 2) # third byte from the end to ensure we only touch the > + # signature value .The location for the perturbation ensures that we are nit: move the '.' left one space

Keeping r+ from keeler.

keeping r+ from keeler (delta of 6.2 to 6.3 ais the patch flag on bugzilla).