Issue8

[necheffa]

This pull request is to fix issue #8.

Originally, I wanted to have goca just place the intermediate .crt under the intermediate's own ca directory. But after some consideration I realized this would break other functionality, for example if we wanted to revoke the intermediate the root CA would not be able to find the intermediate.

Rather than make it difficult to preserve the existing API both in goca proper and the REST interface, I chose to internalize the workaround I developed as a special branch in CA.SignCSR().

[codecov-commenter]

Codecov Report

Merging #10 (837b9d6) into main (5062498) will increase coverage by 0.53%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #10      +/-   ##
==========================================
+ Coverage   65.60%   66.14%   +0.53%     
==========================================
  Files           2        2              
  Lines         314      319       +5     
==========================================
+ Hits          206      211       +5     
  Misses         68       68              
  Partials       40       40              

Impacted Files	Coverage Δ
ca.go	68.87% <100.00%> (+0.65%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5062498...837b9d6. Read the comment docs.

[kairoaraujo]

Thank you, @necheffa, for the PR.

First, sorry for taking long to review. I was busy, and I didn't want to rush on it as your finding is a critical bug.

IMO we should avoid the symbolic link. Let me elaborate more.

One thing I would also apply as another issue is that our current "paths" are only compatible with *Nix systems. We should change it to _storage/storage.go to use path/filepath using filepath.Join("dir1", "dir2", "dir3", "file.txt") to make it cross-platform compatible. [shouldn't be not part of this PR] It doesn't affect the Docker/API version (at the first look), but it would be good practice for the GoCA Package.

Thinking on that, I don't see the symbolic link as a reliable structure. For example, if the user decides to use different filesystems, different OS, or even depending on the storage type, it could have incompatible issues.

As the CA knows, it is an intermediate CA (IsIntermediate()), it should create a copy of the certificate in the {intermediate}/ca/ after singing, in that way, the Intermediate CA will have its autonomy.

Could you share your thoughts?

[necheffa]

@kairoaraujo, no problem. I figured you were busy, plus I have a couple other pots bubbling too.

Regarding path handling, I see you have filed issue #11 and I think that is a good idea. With that being said, I'm not sure that os.Symlink() is necessarily Unix specific. Although, I suppose you could run into trouble with something like network attached storage since os.Symlink() would implement whatever the native symlink mechanism was for the platform GoCA was executed on at that time and then later hosts running other operating systems would not be able to use the resulting $CAPATH.

So I agree, using a symlink leaves us with some ugly problems for heterogeneous environments.

I am not sure the proposed alternative would work though. CA.SignCSR() is passed an x509.CertificateRequest not a goca.CA and I don't think there is currently a way to convert x509.CertificateRequest to goca.CA in order to call IsIntermediate().

But we can get the commonName from x509.CertificateRequest and we might be able to cross-reference this with the result of goca.List(); if the commonName we are signing exists in the list of known CAs then make a copy of the certificate because at that point it is an intermediate.

[necheffa]

@kairoaraujo apologies for the delay on this. But I have reverted the symlink solution as discussed and instead went for a deep-copy of the intermediate CA's certificate instead.