Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do CA checks to verify authentication by default #81

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Do CA checks to verify authentication by default #81

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 15 additions & 14 deletions etc/pam_pkcs11.conf.example.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,21 +92,22 @@ pam_pkcs11 {
support_threads = false;

# Sets the Certificate verification policy.
# "none" Performs no verification, except (!) the signature
# "ca" Does CA check
# "crl_online" Downloads the CRL form the location given by the
# CRL distribution point extension of the certificate
# "crl_offline" Uses the locally stored CRLs
# "crl_auto" Is a combination of online and offline; it first
# tries to download the CRL from a possibly given CRL
# distribution point and if this fails, uses the local
# CRLs
# "signature" Does a signature check to ensure that private
# and public key matches
# "no_signature" The only value that disables signature check.
# "none" Performs only (!) CA and signature checks
# "ca" Does CA check
# "no_ca" The only value that disables CA check
# "crl_online" Downloads the CRL form the location given by the
# CRL distribution point extension of the certificate
# "crl_offline" Uses the locally stored CRLs
# "crl_auto" Is a combination of online and offline; it first
# tries to download the CRL from a possibly given CRL
# distribution point and if this fails, uses the local
# CRLs
# "signature" Does a signature check to ensure that private
# and public key matches
# "no_signature" The only value that disables signature check
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be updated also in the documentation. I initially missed that, but I think the xml source is here:

https://github.com/OpenSC/pam_pkcs11/blob/master/doc/pam_pkcs11.xml#L616

If I see right, the no_signature part is not documented here either.

Having example files as the only documentation is not very good practice.

#
# You can use a combination of ca,crl, and signature flags, or just
# use "none".
# use "none". Use "none,no_ca,no_signature" to disable all checks.
cert_policy = ca,signature;

# What kind of token?
Expand Down Expand Up @@ -140,7 +141,7 @@ pam_pkcs11 {
support_threads = false;
ca_dir = /etc/pam_pkcs11/cacerts;
crl_dir = /etc/pam_pkcs11/crls;
cert_policy = signature;
cert_policy = ca,signature;
}

# Which mappers ( Cert to login ) to use?
Expand Down
8 changes: 4 additions & 4 deletions src/common/cert_vfy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ static X509_STORE * setup_store(cert_policy *policy) {
}
}
/* add needed hash dir pathname entries */
if ( (policy->ca_policy) && (is_dir(policy->ca_dir)>0) ) {
if ( (!policy->no_ca_policy) && (is_dir(policy->ca_dir)>0) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be better readable as

Suggested change
if ( (!policy->no_ca_policy) && (is_dir(policy->ca_dir)>0) ) {
if ( (policy->no_ca_policy==0) && (is_dir(policy->ca_dir)>0) ) {

(similarly the following change)

const char *pt=policy->ca_dir;
if ( strstr(pt,"file:///")) pt+=8; /* strip url if needed */
DBG1("Adding hash dir '%s' to CACERT checks",policy->ca_dir);
Expand All @@ -434,7 +434,7 @@ static X509_STORE * setup_store(cert_policy *policy) {
}
}
/* and add file entries to lookup */
if ( (policy->ca_policy) && (is_file(policy->ca_dir)>0) ) {
if ( (!policy->no_ca_policy) && (is_file(policy->ca_dir)>0) ) {
const char *pt=policy->ca_dir;
if ( strstr(pt,"file:///")) pt+=8; /* strip url if needed */
DBG1("Adding file '%s' to CACERT checks",policy->ca_dir);
Expand Down Expand Up @@ -467,7 +467,7 @@ int verify_certificate(X509 * x509, cert_policy *policy)
X509_STORE_CTX *ctx = NULL;

/* if neither ca nor crl check are requested skip */
if ( (policy->ca_policy==0) && (policy->crl_policy==CRLP_NONE) ) {
if ( (!policy->no_ca_policy==0) && (policy->crl_policy==CRLP_NONE) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be more readable to write this as

Suggested change
if ( (!policy->no_ca_policy==0) && (policy->crl_policy==CRLP_NONE) ) {
if ( (policy->no_ca_policy==1) && (policy->crl_policy==CRLP_NONE) ) {

DBG("Neither CA nor CRL check requested. CertVrfy() skipped");
return 1;
}
Expand All @@ -489,7 +489,7 @@ int verify_certificate(X509 * x509, cert_policy *policy)
#if 0
X509_STORE_CTX_set_purpose(ctx, purpose);
#endif
if (policy->ca_policy) {
if (!policy->no_ca_policy) {
rv = X509_verify_cert(ctx);
if (rv != 1) {
X509_STORE_CTX_free(ctx);
Expand Down
2 changes: 1 addition & 1 deletion src/common/cert_vfy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ typedef enum {
} ocsp_policy_t;

struct cert_policy_st {
int ca_policy;
int no_ca_policy;
int crl_policy;
int no_signature_policy;
const char *ca_dir;
Expand Down
15 changes: 10 additions & 5 deletions src/pam_pkcs11/pam_config.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ static void display_config (void) {
DBG1("crl_dir %s",configuration.policy.crl_dir);
DBG1("nss_dir %s",configuration.policy.nss_dir);
DBG1("support_threads %d",configuration.support_threads);
DBG1("ca_policy %d",configuration.policy.ca_policy);
DBG1("no_ca_policy %d",configuration.policy.no_ca_policy);
DBG1("crl_policy %d",configuration.policy.crl_policy);
DBG1("no_signature_policy %d",configuration.policy.no_signature_policy);
DBG1("ocsp_policy %d",configuration.policy.ocsp_policy);
Expand Down Expand Up @@ -179,7 +179,7 @@ static void parse_config_file(void) {
if ( !strcmp(policy_list->data,"none") ) {
configuration.policy.crl_policy=CRLP_NONE;
configuration.policy.ocsp_policy=OCSP_NONE;
configuration.policy.ca_policy=0;
configuration.policy.no_ca_policy=0;
configuration.policy.no_signature_policy=0;
break;
} else if ( !strcmp(policy_list->data,"crl_auto") ) {
Expand All @@ -191,7 +191,9 @@ static void parse_config_file(void) {
} else if ( !strcmp(policy_list->data,"ocsp_on") ) {
configuration.policy.ocsp_policy=OCSP_ON;
} else if ( !strcmp(policy_list->data,"ca") ) {
configuration.policy.ca_policy=1;
// ignore this setting for legacy reasons
} else if ( !strcmp(policy_list->data,"no_ca") ) {
configuration.policy.no_ca_policy=1;
} else if ( !strcmp(policy_list->data,"signature") ) {
// ignore this setting for legacy reasons
} else if ( !strcmp(policy_list->data,"no_signature") ) {
Expand Down Expand Up @@ -322,7 +324,7 @@ struct configuration_st *pk_configure( int argc, const char **argv ) {
if (strstr(argv[i],"cert_policy=") ) {
if (strstr(argv[i],"none")) {
configuration.policy.crl_policy=CRLP_NONE;
configuration.policy.ca_policy=0;
configuration.policy.no_ca_policy=0;
configuration.policy.no_signature_policy=0;
configuration.policy.ocsp_policy=OCSP_NONE;
}
Expand All @@ -339,7 +341,10 @@ struct configuration_st *pk_configure( int argc, const char **argv ) {
configuration.policy.ocsp_policy=OCSP_ON;
}
if (strstr(argv[i],"ca")) {
configuration.policy.ca_policy=1;
// ignore this setting for legacy reasons
}
if (strstr(argv[i],"no_ca")) {
configuration.policy.no_ca_policy=1;
}
if (strstr(argv[i],"signature")) {
// ignore this setting for legacy reasons
Expand Down
Loading