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

Jump to bottom

CryptoAPI TLS certificate injection #16

Merged
merged 5 commits into from
Jul 30, 2017
Merged

CryptoAPI TLS certificate injection #16

merged 5 commits into from
Jul 30, 2017

Conversation

JeremyRand
Copy link
Member

@JeremyRand JeremyRand commented May 25, 2017 •
edited
Loading

Add the ability to inject TLS certs into CryptoAPI's trust store before replying to DNS queries.

Please review but do not merge yet.

TODO before merging:

  • Make the x509 build script use go generate.
  • Make the x509 build script source go env and use $GOROOT from it.
  • Update the d/ spec to match the current dehydrated certificate format. (It's changed slightly since I submitted the spec.)
  • Look into using Errore instead of Fatal.
  • Fix .gitignore.
  • Squash commits.

@JeremyRand JeremyRand mentioned this pull request May 25, 2017
Merged
2 tasks
@hlandau
Copy link
Member

hlandau commented May 28, 2017

It's up to you but I suspect squashing these would be desirable.

hlandau
hlandau reviewed May 28, 2017
View reviewed changes
backend/backend.go Outdated
// TODO: add callback variable "OnValueReferencedFunc" to backend options so that we don't pollute this function with every hook that we want
// might need to add the other attributes of tx, and sn, to the callback variable for flexibility's sake
// This doesn't normally return errors, but any errors during execution will be logged.
tlshook.DomainValueHookTls(tx.qname, ncv)
Copy link
Member

Choose a reason for hiding this comment

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

style: probably want DomainValueHookTLS

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now.

hlandau
hlandau reviewed May 28, 2017
View reviewed changes
certdehydrate/certdehydrate.go Outdated
import "encoding/binary"
import "fmt"
import "math/big"
import "time"
Copy link
Member

Choose a reason for hiding this comment

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

for future reference, I wrote imports in this style before learning the preferred style:

import (
  "..."
  "..."
)

I switch to this new style as I encounter code with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now.

hlandau
hlandau reviewed May 28, 2017
View reviewed changes
certdehydrate/certdehydrate.go Outdated
// TODO: add a version field
type DehydratedCertificate struct {
PubkeyB64 string
NotBeforeScaledInt int64
Copy link
Member

Choose a reason for hiding this comment

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

avoid Hungarian notation: NotBeforeScaled, not NotBeforeScaledInt. Use comments on individual fields to document the format.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now.

hlandau
hlandau reviewed May 28, 2017
View reviewed changes
certdehydrate/certdehydrate.go
return serialHash.Sum(nil)[0:19], nil
}

func (dehydrated DehydratedCertificate) String() string {
Copy link
Member

Choose a reason for hiding this comment

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

You could use encoding/json to do this: serialize a []interface{} containing the values. This works, though it could have a vulnerability if bad data gets into the base64 fields somehow. At any rate, there's some whitespace in here you can remove to minimise the size.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now.

hlandau
hlandau reviewed May 28, 2017
View reviewed changes
certdehydrate/certdehydrate.go Outdated

func ParseDehydratedCert(data interface{}) (*DehydratedCertificate, error) {
dehydrated, ok := data.([]interface{})
if ! ok {
Copy link
Member

Choose a reason for hiding this comment

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

Errant space after !.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now.

hlandau
hlandau reviewed May 28, 2017
View reviewed changes
certdehydrate/certdehydrate.go Outdated
signatureBytes := cert.Signature
signatureB64 := base64.StdEncoding.EncodeToString(signatureBytes)

result := DehydratedCertificate {
Copy link
Member

Choose a reason for hiding this comment

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

DehydratedCertificate{ (no space) is standard Go style IIRC.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now.

hlandau
hlandau reviewed May 28, 2017
View reviewed changes
certdehydrate/certdehydrate.go Outdated
}

return &result, nil

Copy link
Member

Choose a reason for hiding this comment

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

Empty lines after return.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now.

hlandau
hlandau reviewed May 28, 2017
View reviewed changes
certdehydrate/certdehydrate.go Outdated
return nil, fmt.Errorf("Dehydrated cert signature must be valid base64: %s", err)
}

template := x509.Certificate {
Copy link
Member

Choose a reason for hiding this comment

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

Space before {

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now.

hlandau
hlandau reviewed May 28, 2017
View reviewed changes
certdehydrate/certdehydrate_test.go Outdated
"github.com/hlandau/ncdns/certdehydrate"
)

func TestDehydratedCertIdentityOperation(t *testing.T){
Copy link
Member

Choose a reason for hiding this comment

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

No space before {

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now.

hlandau
hlandau reviewed May 28, 2017
View reviewed changes
certdehydrate/certdehydrate_test.go Outdated
}

// Test to make sure that rehydrating and then dehydrating a cert doesn't change it.
if ! reflect.DeepEqual(dehydrated, dehydrated2) {
Copy link
Member

Choose a reason for hiding this comment

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

Space after !

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now.

hlandau
hlandau reviewed May 28, 2017
View reviewed changes
certinject/cryptoapi_windows.go Outdated
// Format documentation of Microsoft's "Certificate Registry Blob":

// 5c 00 00 00 // propid
// 01 00 00 00 // unknown
Copy link
Member

@hlandau hlandau May 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Irrelevant, but it's probably a version or flags field.

Copy link
Member Author

Choose a reason for hiding this comment

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

Quite possibly. None of the reverse-engineered documentation that I found explained what the field was for or what format it used. In any event its value seems to be constant everywhere, so reverse-engineering what it's for doesn't seem necessary.

hlandau
hlandau reviewed May 28, 2017
View reviewed changes
certinject/cryptoapi_windows.go Outdated

// 5c 00 00 00 // propid
// 01 00 00 00 // unknown
// 04 00 00 00 // size (little endian because Microsoft engineers suck at their jobs)
Copy link
Member

Choose a reason for hiding this comment

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

Eh, little endian is fine. Windows is usually little endian and the adoption of big endian for network order was largely based on the fact that servers at the time (POWER, SPARC, etc.) were largely big endian.

Little endian has won (x86, ARM) and I'd probably use it for a binary network protocol if I were designing one today.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed comment.

hlandau
hlandau reviewed May 28, 2017
View reviewed changes
certinject/cryptoapi_windows.go Outdated
certLength := len(derBytes)

// Header for a stripped Windows Certificate Registry Blob
certBlobHeader := []byte{0x20, 0, 0, 0, 0x01, 0, 0, 0, byte( (certLength >> 0) & 0xFF), byte( (certLength >> 8) & 0xFF), byte( (certLength >> 16) & 0xFF), byte( (certLength >> 24) & 0xFF) }
Copy link
Member

Choose a reason for hiding this comment

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

Personally I'd use encoding/binary to store the fields into a fixed-length byte array. This is probably faster but it's less clear. Up to you.

Copy link
Member Author

Choose a reason for hiding this comment

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

For this use case, speed is very important. Feel free to create a separate issue for this, and I'll do benchmarking later to see which is faster (and I'll use whatever wins that test).

hlandau
hlandau reviewed May 28, 2017
View reviewed changes
certinject/cryptoapi_windows.go Outdated
// Open up the cert store.
certStoreKey, err := registry.OpenKey(cryptoApiCertStoreRegistryBase, cryptoApiCertStoreRegistryKey, registry.ALL_ACCESS)
if err != nil {
log.Fatal(err)
Copy link
Member

@hlandau hlandau May 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

you probably want log.Fatale here with a short message like "couldn't open certificate store" (and again below), otherwise it can be hard to figure out where an error comes from.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I'm trying to remember why I used Fatal instead of Error and I suspect I made a mistake. Do you agree that Errore makes more sense than Fatale?

hlandau
hlandau reviewed May 28, 2017
View reviewed changes
certinject/cryptoapi_windows.go Outdated
}

// for all certs in the cert store
for _,subKeyName := range subKeys {
Copy link
Member

Choose a reason for hiding this comment

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

missing space after _,

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now.

hlandau
hlandau reviewed May 28, 2017
View reviewed changes
certinject/file.go Outdated
pemBytes := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: derBytes})
err := ioutil.WriteFile(fileName, pemBytes, 0644)
if err != nil {
log.Error("Error writing cert!")
Copy link
Member

Choose a reason for hiding this comment

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

log.Errore(err, "Error wrtiting cert")

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now

hlandau
hlandau reviewed May 28, 2017
View reviewed changes
x509_build/install.sh Outdated
@@ -0,0 +1,4 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

/bin/sh will suffice here, some systems will lack bash or have it at another location.

Copy link
Member Author

Choose a reason for hiding this comment

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

The main reason I used bash is that I don't have a mental model of which constructs are bash-specific and which are also present in sh, which makes it risky that I'll accidentally use a bash construct in sh if I habitually use sh. What systems lack Bash? I was under the impression that basically all GNU systems have Bash; are you talking about non-GNU things like Busybox?

@hlandau
Copy link
Member

hlandau commented May 28, 2017

Made some minor comments, mostly just stylistic comments for now.

@JeremyRand
Copy link
Member Author

AFAICT the Travis CI failure is due to #19 .

@JeremyRand
Copy link
Member Author

It's up to you but I suspect squashing these would be desirable.

I'll squash this PR before it's merged, but after it passes review.

@JeremyRand
Copy link
Member Author

JeremyRand commented Jun 6, 2017 via email

@JeremyRand JeremyRand force-pushed the tls-import-certs-go-registry-stable branch from 2ce01fb to f9adde0 Compare June 10, 2017 11:55
@JeremyRand JeremyRand closed this Jun 10, 2017
@JeremyRand JeremyRand reopened this Jun 10, 2017
@JeremyRand
Copy link
Member Author

JeremyRand commented Jun 10, 2017 via email

@JeremyRand JeremyRand mentioned this pull request Jun 13, 2017
Merged
@JeremyRand
Copy link
Member Author

JeremyRand commented Jun 14, 2017 via email

@JeremyRand JeremyRand changed the title (WIP) CryptoAPI TLS certificate injection CryptoAPI TLS certificate injection Jun 14, 2017
@JeremyRand JeremyRand force-pushed the tls-import-certs-go-registry-stable branch from a101610 to deffffa Compare June 20, 2017 06:54
@JeremyRand
Copy link
Member Author

JeremyRand commented Jun 20, 2017 via email

@hlandau
Copy link
Member

hlandau commented Jun 20, 2017

You'd better remove .gitignore.

@JeremyRand
Copy link
Member Author

JeremyRand commented Jun 20, 2017 via email

hlandau
hlandau approved these changes Jun 20, 2017
View reviewed changes
Copy link
Member

@hlandau hlandau left a comment

Choose a reason for hiding this comment

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

ACK

@JeremyRand
Copy link
Member Author

JeremyRand commented Jun 20, 2017 via email

@hlandau
Copy link
Member

hlandau commented Jul 2, 2017

ACK 2e6ee03

@JeremyRand JeremyRand force-pushed the tls-import-certs-go-registry-stable branch from 2e6ee03 to d67066b Compare July 8, 2017 02:20
@JeremyRand
Copy link
Member Author

JeremyRand commented Jul 8, 2017 via email

hlandau
hlandau reviewed Jul 8, 2017
View reviewed changes
tlshook/tlshook.go Outdated
// TODO: test this code.
// since this code has not been tested yet, it's disabled for safety reasons.
//if len(port.TLSA) > 0 {
if false {
Copy link
Member

Choose a reason for hiding this comment

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

?

Too much commented out code here. Not in merge condition.

@JeremyRand
Copy link
Member Author

JeremyRand commented Jul 13, 2017 via email

@hlandau
Copy link
Member

hlandau commented Jul 22, 2017

Needs rebasing.

@JeremyRand JeremyRand force-pushed the tls-import-certs-go-registry-stable branch from e092f32 to 3202450 Compare July 26, 2017 01:33
@JeremyRand
Copy link
Member Author

JeremyRand commented Jul 26, 2017 via email

@hlandau
Copy link
Member

hlandau commented Jul 26, 2017

ACK 3202450 and the commits on which it builds.

@JeremyRand JeremyRand closed this Jul 28, 2017
@JeremyRand JeremyRand reopened this Jul 28, 2017
@JeremyRand JeremyRand force-pushed the tls-import-certs-go-registry-stable branch from 3202450 to aec2cc2 Compare July 28, 2017 02:33
@JeremyRand
Travis: Disable gometalinter warnings on the portion of x509 that is …
978116d 
…copied verbatim from the Go standard library.
@JeremyRand JeremyRand force-pushed the tls-import-certs-go-registry-stable branch from aec2cc2 to 978116d Compare July 28, 2017 02:38
@JeremyRand
Copy link
Member Author

JeremyRand commented Jul 28, 2017 via email

@hlandau
Copy link
Member

hlandau commented Jul 28, 2017

ACK 271a0c7

@JeremyRand JeremyRand merged commit 271a0c7 into namecoin:master Jul 30, 2017
JeremyRand added a commit that referenced this pull request Jul 30, 2017
@JeremyRand
Merge #16: CryptoAPI TLS certificate injection
dbc5d47 
271a0c7 tlshook: Fix linter warning about shadowed variable. (JeremyRand)
978116d Travis: Disable gometalinter warnings on the portion of x509 that is copied verbatim from the Go standard library. (JeremyRand)
05afcd4 tlshook: Remove unused imports. (JeremyRand)
81fb477 tlshook: Removed commented-out code for non-dehydrated certificates; I plan to re-add that code once it's properly tested. (JeremyRand)
e16ad6f TLS dehydrated certificate injection for CryptoAPI trust store (triggered by hooking DNS lookups). (JeremyRand)

Pull request description:

  Add the ability to inject TLS certs into CryptoAPI's trust store before replying to DNS queries.

  Please review but do not merge yet.

  TODO before merging:

  - [x] Make the x509 build script use `go generate`.
  - [x] Make the x509 build script source `go env` and use `$GOROOT` from it.
  - [x] Update the `d/` spec to match the current dehydrated certificate format.  (It's changed slightly since I submitted the spec.)
  - [x] Look into using Errore instead of Fatal.
  - [x] Fix `.gitignore`.
  - [x] Squash commits.

Tree-SHA512: 1ce4e650e142aa1630f51b09497d85ad0626ae46ccc63c2e72fafa97d99bf340b3583db5ac76b5cc339228e56be8e9db673348d2d8f1f6173f1bca5306971629
@JeremyRand JeremyRand deleted the tls-import-certs-go-registry-stable branch November 8, 2017 16:21
JeremyRand added a commit that referenced this pull request Mar 12, 2018
@JeremyRand
Merge #17: NSS TLS certificate injection
49b1818 
375ff45 certinject: NSS: Add an internal test. (JeremyRand)
ead7a20 certinject: NSS: Improve error handling. (JeremyRand)
145d1e3 certinject: Fix various issues found by static analysis. (JeremyRand)
2c8b5fe certinject: NSS improvements, now works on arbitrary NSS cert store directories. (JeremyRand)
e5c7c09 certinject: add support for the shared NSS trust store on GNU/Linux systems. (JeremyRand)

Pull request description:

  Extend #16 to support the user's shared NSS trust store on GNU/Linux systems.

  Please review but do not merge yet.

  TODO before merging:

  - [x] Get #16 merged.
  - [x] Figure out what to do in the case where ncdns isn't run by the same user as the owner of the NSS database.  Presumably it makes sense to run ncdns under its own user.  Should we require a config option that lists the users whose NSS databases are written to?

  Other issue to discuss:

  Writing to the NSS database with `certutil` is really slow, I'm seeing ~700ms latency added by this.  Is there a faster way to do it?  If we try to handle multiple NSS databases (one per user), this could easily cause DNS timeouts.  Using the system NSS database should be possible, but it would be unsafe for users who haven't installed the HPKP pin into Chromium.

Tree-SHA512: d35fcb44e6c09d6654140de8cf378b0b7523ac19d63d007064db14d5c84cd2178cad95d348baa3234843d215fb563185b98ced33c3e876876d8d42a01ba4e6a7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hlandau hlandau hlandau approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JeremyRand @hlandau