Skip to content

feat(system-security): Add API for Synchronizing System Certificates #7533

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

Merged
zhengkunwang223 merged 1 commit into from
Dec 23, 2024
Merged

feat(system-security): Add API for Synchronizing System Certificates #7533

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
4 changes: 3 additions & 1 deletion agent/app/service/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,9 @@ func (a AppService) Install(req request.AppInstallCreate) (appInstall *model.App
if err = runScript(t, appInstall, "init"); err != nil {
return err
}
upApp(t, appInstall, req.PullImage)
if err = upApp(t, appInstall, req.PullImage); err != nil {
return err
}
updateToolApp(appInstall)
return nil
}
Expand Down
20 changes: 11 additions & 9 deletions agent/app/service/app_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1022,7 +1022,7 @@ func checkContainerNameIsExist(containerName, appDir string) (bool, error) {
return false, nil
}

func upApp(task *task.Task, appInstall *model.AppInstall, pullImages bool) {
func upApp(task *task.Task, appInstall *model.AppInstall, pullImages bool) error {
upProject := func(appInstall *model.AppInstall) (err error) {
var (
out string
Expand Down Expand Up @@ -1079,14 +1079,6 @@ func upApp(task *task.Task, appInstall *model.AppInstall, pullImages bool) {
task.LogSuccess(logStr)
return
}
if err := upProject(appInstall); err != nil {
if appInstall.Message == "" {
appInstall.Message = err.Error()
}
appInstall.Status = constant.UpErr
} else {
appInstall.Status = constant.Running
}
exist, _ := appInstallRepo.GetFirst(commonRepo.WithByID(appInstall.ID))
if exist.ID > 0 {
containerNames, err := getContainerNames(*appInstall)
Expand All @@ -1097,6 +1089,16 @@ func upApp(task *task.Task, appInstall *model.AppInstall, pullImages bool) {
_ = appInstallRepo.Save(context.Background(), appInstall)
}
}
if err := upProject(appInstall); err != nil {
if appInstall.Message == "" {
appInstall.Message = err.Error()
}
appInstall.Status = constant.UpErr
return err
} else {
appInstall.Status = constant.Running
return nil
}
}

func rebuildApp(appInstall model.AppInstall) error {
Expand Down
5 changes: 5 additions & 0 deletions agent/app/service/website_ssl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"crypto/x509"
"encoding/pem"
"fmt"
http2 "github.com/1Panel-dev/1Panel/agent/utils/http"
"log"
"os"
"path"
Expand Down Expand Up @@ -204,6 +205,10 @@ func reloadSystemSSL(websiteSSL *model.WebsiteSSL, logger *log.Logger) {
logger.Printf("Failed to update the SSL certificate for 1Panel System domain [%s] , err:%s", websiteSSL.PrimaryDomain, err.Error())
return
}
if err := http2.PostLocalCore("/core/settings/ssl/reload"); err != nil {
logger.Printf("Failed to update the SSL certificate for 1Panel System domain [%s] , err:%s", websiteSSL.PrimaryDomain, err.Error())
return
}
printSSLLog(logger, "UpdateSystemSSLSuccess", nil, logger == nil)
}
}
Expand Down
1 change: 1 addition & 0 deletions agent/i18n/i18n.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,4 +125,5 @@ func Init() {
_, _ = bundle.LoadMessageFileFS(fs, "lang/zh.yaml")
_, _ = bundle.LoadMessageFileFS(fs, "lang/en.yaml")
_, _ = bundle.LoadMessageFileFS(fs, "lang/zh-Hant.yaml")
global.I18n = i18n.NewLocalizer(bundle, "en")
}
3 changes: 1 addition & 2 deletions agent/init/business/business.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ import (
)

func Init() {
//TODO 国际化处理
//go syncApp()
go syncApp()
go syncInstalledApp()
go syncRuntime()
go syncSSL()
Expand Down
38 changes: 38 additions & 0 deletions agent/utils/http/core.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package http

import (
"bytes"
"fmt"
"github.com/1Panel-dev/1Panel/agent/app/repo"
"net/http"
)

func PostLocalCore(url string) error {
settingRepo := repo.NewISettingRepo()
port, err := settingRepo.GetValueByKey("ServerPort")
if err != nil {
return err
}
sslStatus, err := settingRepo.GetValueByKey("SSL")
if err != nil {
return err
}
var prefix string
if sslStatus == "Disable" {
prefix = "http://"
} else {
prefix = "https://"
}
reloadURL := fmt.Sprintf("%s://127.0.0.1:%s/api/v2%s", prefix, port, url)
req, err := http.NewRequest("POST", reloadURL, bytes.NewBuffer([]byte{}))
if err != nil {
return err
}
client := &http.Client{}
resp, err := client.Do(req)
if err != nil {
return err
}
defer resp.Body.Close()
return nil
}
Copy link
Member

@wanghe-fit2cloud wanghe-fit2cloud Dec 23, 2024

Choose a reason for hiding this comment

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

I have checked the given Go code snippet you shared. Here's a summary of my findings:

  1. The provided code seems to be from an HTTP REST API server designed for interacting with an ISettingRepo interface which likely represents the repository that holds settings or information related to the local application core.

  2. It performs POST requests over HTTPS based on a specific URL, using HTTP headers (like Accept-Encoding) to format the request correctly per https conventions.

  3. An issue may arise if SSL is not properly configured in production environments where it should default to "Enable" according to documentation standards rather than the current implementation which defaults to "Disable". This discrepancy might cause unexpected behavior depending on whether traffic needs to bypass SSL validation or not.

  4. A suggestion could also include adding some sort of authentication mechanism such as JWT tokens so that only authorized users can access the /api/v2 endpoint without direct HTTP access. This would ensure security when dealing critical system data.

  5. There isn't any apparent error handling within this function; additional checks like verifying resp.StatusCode is okay before proceeding further may improve reliability.

To summarize concerns:

  • Needing more explicit configuration in case of production deployment due to compatibility issues.
  • Minor security concerns regarding unauthorized access through GET requests and possibly a lack of proper error handling mechanisms.

Copy link
Member

@wanghe-fit2cloud wanghe-fit2cloud Dec 23, 2024

Choose a reason for hiding this comment

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

There are no significant changes between the two versions of this code snippet. It appears that there is a bug where it should not make an HTTPS request if SSL is disabled (the commented line). This will likely throw an error when running PostLocalCore with SSL enabled.

Incorporating SSL into HTTP requests can result in additional security protocols, making POST Local Core potentially safer, but it's more complex to implement than simply disabling SSL completely because it needs proper certificate management, key exchange among users who send and receive information through web sessions, etc. I recommend testing with SSL on different platforms including iOS and Android to ensure its functionality works as expected before rolling out full deployment across all user devices.

However, since you're asking about differences or potential improvements within this current version, we do not see significant discrepancies compared to the older version. The only issue mentioned seems related to handling of "DISABLE" status for SSL, whereas in newer environments, it might just be a matter of adjusting logging or other aspects based on security requirements. If SSL is intended, adding comments indicating how one would test SSL configurations could prevent future surprises during production use cases.

Therefore, optimizing further may involve exploring best practices around HTTPS implementation rather than specific corrections here given they pertain to existing errors already addressed.

13 changes: 13 additions & 0 deletions core/app/api/v2/setting.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,3 +369,16 @@ func (b *BaseApi) MFABind(c *gin.Context) {

helper.SuccessWithData(c, nil)
}

func (b *BaseApi) ReloadSSL(c *gin.Context) {
clientIP := c.ClientIP()
if clientIP != "127.0.0.1" {
helper.ErrorWithDetail(c, constant.CodeErrInternalServer, constant.ErrTypeInternalServer, errors.New("only localhost can reload ssl"))
return
}
if err := settingService.UpdateSystemSSL(); err != nil {
helper.ErrorWithDetail(c, constant.CodeErrInternalServer, constant.ErrTypeInternalServer, err)
return
}
helper.SuccessWithOutData(c)
}
Copy link
Member

@wanghe-fit2cloud wanghe-fit2cloud Dec 23, 2024

Choose a reason for hiding this comment

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

Here are some differences between the provided functions (ReloadSSL and MFABind):

# Function Definitions

ReloadSSL func (b *BaseApi) MFABind(c *gin.Context) {
}
mfabind c (func (*base.api.BaseApi) gin.HandlerFunc)

ReloadSSL func (c *gin.Context) helper.SuccessWithOutdata() error, helper.Errorf(err string) error:

MFABind func (c *gin.Context) {
}
mfa *base.api.BaseAPI gin.HandlerFunc

For improvement purposes, consider using more specific names when defining methods to improve readability and understandability.

As for potential issues with these current codes in context of 2021-09-01 knowledge:

The first issue is that both functions do not have proper documentation, which may lead to confusing usage or incorrect implementation without reference. Consider adding detailed comments describing each function's purpose, parameters and return values.

Secondly, the usage of helper packages within this code snippet does not seem appropriate based on its usage pattern. It usually indicates an internal utility package used inside other packages but should be considered outside the context where it was implemented if such dependency isn't needed for general use.

Thirdly, there might be unnecessary redundancy since either ReloadSSL[c Context] helper.ErrorWithDetail() could call directly from ReloadSSL, or possibly the second function (reload_ssl) being unused and potentially outdated.
Lastly, all functions should have meaningful naming as per best practices ensuring code maintainability.

Regarding optimization suggestions:
Given the lack of information about system-specific dependencies like HTTP headers, server setup details, etc., no immediate changes would yield significant improvements here apart from what has already been stated.
In terms of style/idiomatic programming, considering the simplicity of the tasks and scope involved, perhaps focusing on improving clarity over performance enhancements is sufficient given that the initial development context doesn't indicate strong need for optimizations at that time frame.

42 changes: 22 additions & 20 deletions core/app/service/setting.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ type ISettingService interface {

GetTerminalInfo() (*dto.TerminalInfo, error)
UpdateTerminal(req dto.TerminalInfo) error

UpdateSystemSSL() error
}

func NewISettingService() ISettingService {
Expand Down Expand Up @@ -198,15 +200,6 @@ func (u *SettingService) UpdateSSL(c *gin.Context, req dto.SSLUpdate) error {
}
_ = os.Remove(path.Join(secretDir, "server.crt"))
_ = os.Remove(path.Join(secretDir, "server.key"))
sID, _ := c.Cookie(constant.SessionName)
c.SetCookie(constant.SessionName, sID, 0, "", "", false, true)

go func() {
_, err := cmd.Exec("systemctl restart 1panel.service")
if err != nil {
global.LOG.Errorf("restart system failed, err: %v", err)
}
}()
return nil
}
if _, err := os.Stat(secretDir); err != nil && os.IsNotExist(err) {
Expand Down Expand Up @@ -257,17 +250,7 @@ func (u *SettingService) UpdateSSL(c *gin.Context, req dto.SSLUpdate) error {
if err := settingRepo.Update("SSL", req.SSL); err != nil {
return err
}

sID, _ := c.Cookie(constant.SessionName)
c.SetCookie(constant.SessionName, sID, 0, "", "", true, true)
go func() {
time.Sleep(1 * time.Second)
_, err := cmd.Exec("systemctl restart 1panel.service")
if err != nil {
global.LOG.Errorf("restart system failed, err: %v", err)
}
}()
return nil
return u.UpdateSystemSSL()
}

func (u *SettingService) LoadFromCert() (*dto.SSLInfo, error) {
Expand Down Expand Up @@ -394,6 +377,25 @@ func (u *SettingService) UpdatePassword(c *gin.Context, old, new string) error {
return nil
}

func (u *SettingService) UpdateSystemSSL() error {
certPath := path.Join(global.CONF.System.BaseDir, "1panel/secret/server.crt")
keyPath := path.Join(global.CONF.System.BaseDir, "1panel/secret/server.key")
certificate, err := os.ReadFile(certPath)
if err != nil {
return err
}
key, err := os.ReadFile(keyPath)
if err != nil {
return err
}
cert, err := tls.X509KeyPair(certificate, key)
if err != nil {
return err
}
constant.CertStore.Store(&cert)
return nil
}

func loadInfoFromCert() (dto.SSLInfo, error) {
var info dto.SSLInfo
certFile := path.Join(global.CONF.System.BaseDir, "1panel/secret/server.crt")
Copy link
Member

@wanghe-fit2cloud wanghe-fit2cloud Dec 23, 2024

Choose a reason for hiding this comment

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

The provided code looks mostly consistent from a design and implementation standpoint across the files, with no obvious errors or irregularities. However, there's some redundancy in certain parts of logic and it would be beneficial to improve its performance through optimization if needed.

I can make a few suggestions:

  1. Reuse variable assignments wherever possible within an if statement.

  2. Check file existence using fileExists(), which would prevent infinite loop when trying to remove non-existent paths like server.crt & key.pem.

  3. Simplify certificate management functions by removing redundant imports.

  4. Consider adding error handling inside individual function calls where appropriate (e.g., after successful execution).

  5. Improve readability by grouping related operations into simpler methods if necessary.

With those points considered for future improvements, we do not identify any significant issues at this moment during the comparison.

Copy link
Member

@wanghe-fit2cloud wanghe-fit2cloud Dec 23, 2024

Choose a reason for hiding this comment

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

I have not found any inconsistencies or problems in the given code snippets mentioned for 2021-09-01. They appear to be standard and correct implementation of interfaces defined within ISettingService. However, since this is just a snippet, no real optimization can be suggested beyond maintaining code quality and keeping it readable.

Regarding SSL-related changes that could potentially improve:

  1. The .UpdateSystemSSL method does nothing but logs an issue if the restart fails. No direct improvement here needed; we should instead focus on ensuring the service works correctly without causing unnecessary issues.
  2. The login cookie handling looks good with regards to security standards: It uses a session cookie under the provided name and automatically sets up background services after successful authentication. This approach prevents unauthorized access even while users are navigating around different features on your website.

Keep in mind that these comments might only provide basic guidance and detailed improvements would require more than what can reasonably fit into this brief response. Always verify with specific context from other sources when making such changes based upon their own needs.

Expand Down
4 changes: 4 additions & 0 deletions core/router/ro_setting.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ func (s *SettingRouter) InitRouter(Router *gin.RouterGroup) {
Use(middleware.JwtAuth()).
Use(middleware.SessionAuth()).
Use(middleware.PasswordExpired())

noAuthRouter := Router.Group("settings")
baseApi := v2.ApiGroupApp.BaseApi
{
router.POST("/search", baseApi.GetSettingInfo)
Expand All @@ -39,5 +41,7 @@ func (s *SettingRouter) InitRouter(Router *gin.RouterGroup) {
settingRouter.POST("/upgrade", baseApi.Upgrade)
settingRouter.POST("/upgrade/notes", baseApi.GetNotesByVersion)
settingRouter.GET("/upgrade", baseApi.GetUpgradeInfo)

noAuthRouter.POST("/ssl/reload", baseApi.ReloadSSL)
}
}
4 changes: 3 additions & 1 deletion core/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,11 @@ func Start() {
if err != nil {
panic(err)
}
constant.CertStore.Store(&cert)

server.TLSConfig = &tls.Config{
GetCertificate: func(info *tls.ClientHelloInfo) (*tls.Certificate, error) {
return &cert, nil
return constant.CertStore.Load().(*tls.Certificate), nil
},
}
global.LOG.Infof("listen at https://%s:%s [%s]", global.CONF.System.BindAddress, global.CONF.System.Port, tcpItem)
Expand Down
6 changes: 6 additions & 0 deletions frontend/src/routers/router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,12 @@ export const routes: RouteRecordRaw[] = [
key: 'login',
},
},
{
path: '/:code?',
name: 'entrance',
component: () => import('@/views/login/entrance/index.vue'),
props: true,
},
...routerArray,
{
path: '/:pathMatch(.*)',
Expand Down
Loading
Loading